⚡ Bolt: Optimize GetRackCount with O(1) lookup#175
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The optimization of GetRackCount from O(N) to O(1) is a performance improvement; however, the current implementation has significant architectural and stability risks. Specifically, the use of a hardcoded array index to access internal game data is brittle and susceptible to breaking in future game updates.
Furthermore, the fallback mechanism to FindObjectsOfType may cause frame stutters in states where NetworkMap is uninitialized (e.g., loading screens), contradicting the goal of the optimization. While Codacy indicates the PR is up to standards, the complexity increase in GregFacilityModule.cs reflects these new conditional branches and indices.
About this PR
- The optimization relies on a hardcoded array index (2) from the
Il2Cpp.NetworkMaplibrary. This creates a high risk of failure if the external library's data structure changes in a patch, as the code cannot programmatically verify if index 2 still represents 'racks'. - No unit or integration tests were included to verify the O(1) logic or the fallback paths. Given the reliance on internal game state, testing is critical to prevent regressions.
Test suggestions
- Return rack count from NetworkMap index 2 when the instance and array are valid.
- Fallback to FindObjectsOfType when NetworkMap.instance is null.
- Fallback to FindObjectsOfType when GetNumberOfDevices returns an array with length less than 3.
- Fallback to FindObjectsOfType when GetNumberOfDevices returns a null array.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Return rack count from NetworkMap index 2 when the instance and array are valid.
2. Fallback to FindObjectsOfType when NetworkMap.instance is null.
3. Fallback to FindObjectsOfType when GetNumberOfDevices returns an array with length less than 3.
4. Fallback to FindObjectsOfType when GetNumberOfDevices returns a null array.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| } | ||
|
|
||
| // Fallback during uninitialized game states | ||
| return UnityEngine.Object.FindObjectsOfType<global::Il2Cpp.Rack>().Length; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The fallback FindObjectsOfType is an O(N) operation that scans all GameObjects. When NetworkMap.instance is null (e.g., main menu), this will execute repeatedly and cause stutters. Since NetworkMap is the authority, a null instance should likely default to returning 0 to avoid the expensive scan.
| internal GregFacilityModule(GregApiContext ctx) => _ctx = ctx; | ||
|
|
||
| // Index 2 represents racks in GetNumberOfDevices array (0: servers, 1: switches) | ||
| private const int DEVICE_INDEX_RACKS = 2; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The use of a hardcoded index 2 for DEVICE_INDEX_RACKS is brittle. If the game's internal NetworkMap structure changes, this logic will return incorrect device counts without a runtime crash. Search the global::Il2Cpp namespace for an Enum or constants related to NetworkMap.GetNumberOfDevices indices and refactor to use a named constant if available.
💡 What: Replaced
FindObjectsOfType<Rack>()inGetRackCount()with O(1) array access viaIl2Cpp.NetworkMap.instance.GetNumberOfDevices(). Added fallback to original method when NetworkMap instance is null (e.g., during main menu).🎯 Why:
FindObjectsOfTypeis O(N) and scans the entire object hierarchy, which causes noticeable lag and CPU overhead during gameplay when frequently called, especially as device counts grow.📊 Impact: Reduces computational complexity from O(N) over all scene objects to O(1) constant time, saving significant CPU cycles and reducing main thread blocking.
🔬 Measurement: Profile the application during late-game scenarios; CPU time spent in
GetRackCountshould drop to effectively zero without degrading accuracy.PR created automatically by Jules for task 7197781531364853088 started by @mleem97