⚡ Bolt: Optimize GetRackCount to use O(1) NetworkMap lookup#181
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
This PR optimizes the GetRackCount method to use an O(1) lookup via NetworkMap, which is a significant improvement for late-game performance. However, there is a contradiction between the implementation requirements and the performance goals: the current fallback to FindObjectsOfType re-introduces the O(N) complexity and GC pressure that this optimization aims to solve. I recommend returning 0 in uninitialized states instead of performing a hierarchy scan.
Additionally, GregFacilityModule.cs shows an increase in complexity with zero test coverage for the new logic. No unit or integration tests were provided to verify the safety of the array indexing or the singleton access. While Codacy indicates the PR is up to standards, the lack of automated verification for a high-frequency API call is a risk.
About this PR
- This PR lacks unit or integration tests to verify the new O(1) lookup path and the fallback logic. Given this impacts high-frequency API calls, automated verification is recommended to prevent regressions or index-out-of-range errors.
Test suggestions
- Verify GetRackCount returns the correct value from NetworkMap.instance.GetNumberOfDevices() when the game is fully initialized.
- Verify GetRackCount returns 0 (or falls back safely) when NetworkMap.instance is null.
- Verify GetRackCount returns 0 (or falls back safely) when GetNumberOfDevices() returns null or an empty array.
- Verify GetRackCount returns 0 (or falls back safely) when the device array length is shorter than the RACK index (2).
- Ensure the optimized O(1) path is covered by unit tests to prevent regressions in GregFacilityModule complexity.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify GetRackCount returns the correct value from NetworkMap.instance.GetNumberOfDevices() when the game is fully initialized.
2. Verify GetRackCount returns 0 (or falls back safely) when NetworkMap.instance is null.
3. Verify GetRackCount returns 0 (or falls back safely) when GetNumberOfDevices() returns null or an empty array.
4. Verify GetRackCount returns 0 (or falls back safely) when the device array length is shorter than the RACK index (2).
5. Ensure the optimized O(1) path is covered by unit tests to prevent regressions in GregFacilityModule complexity.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| public int GetRackCount() | ||
| { | ||
| if (global::Il2Cpp.NetworkMap.instance != null) | ||
| { | ||
| var counts = global::Il2Cpp.NetworkMap.instance.GetNumberOfDevices(); | ||
| if (counts != null && counts.Length > DEVICE_INDEX_RACKS) | ||
| { | ||
| return counts[DEVICE_INDEX_RACKS]; | ||
| } | ||
| } | ||
|
|
||
| // Fallback for uninitialized states (e.g. main menu) | ||
| return UnityEngine.Object.FindObjectsOfType<global::Il2Cpp.Rack>().Length; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: To maximize performance in IL2CPP and maintain the O(1) goal:
- Cache
global::Il2Cpp.NetworkMap.instancein a local variable to avoid multiple managed-to-native transitions. - Remove the
FindObjectsOfTypefallback. If the manager is missing, returning0is more efficient and aligns with the expected state.
| public int GetRackCount() | |
| { | |
| if (global::Il2Cpp.NetworkMap.instance != null) | |
| { | |
| var counts = global::Il2Cpp.NetworkMap.instance.GetNumberOfDevices(); | |
| if (counts != null && counts.Length > DEVICE_INDEX_RACKS) | |
| { | |
| return counts[DEVICE_INDEX_RACKS]; | |
| } | |
| } | |
| // Fallback for uninitialized states (e.g. main menu) | |
| return UnityEngine.Object.FindObjectsOfType<global::Il2Cpp.Rack>().Length; | |
| } | |
| public int GetRackCount() | |
| { | |
| var networkMap = global::Il2Cpp.NetworkMap.instance; | |
| if (networkMap != null) | |
| { | |
| var counts = networkMap.GetNumberOfDevices(); | |
| if (counts != null && counts.Length > DEVICE_INDEX_RACKS) | |
| { | |
| return counts[DEVICE_INDEX_RACKS]; | |
| } | |
| } | |
| return 0; | |
| } |
| **Action:** Always prefer using global singleton collections managed by the game over calling `FindObjectsOfType<T>`. For example, use `Il2Cpp.NetworkMap.instance.servers` to get servers, `Il2Cpp.NetworkMap.instance.switches` for switches and `Il2Cpp.TechnicianManager.instance.technicians` to get technicians. Ensure null checks are present. | ||
| ## 2025-05-20 - Optimizing IL2CPP Lookups with Indexed Arrays | ||
| **Learning:** Calling `UnityEngine.Object.FindObjectsOfType<T>().Length` to get counts of common devices like Racks, Servers, and Switches is an O(N) operation that creates unnecessary garbage and CPU spikes, especially when called frequently. | ||
| **Action:** Instead of `FindObjectsOfType`, use `Il2Cpp.NetworkMap.instance.GetNumberOfDevices()`, which returns an array where indices map to counts (e.g., 0 = Servers, 1 = Switches, 2 = Racks) for O(1) count retrieval. Always provide a fallback to `FindObjectsOfType` in case `NetworkMap.instance` is null (like in the main menu) and check array bounds using descriptive constants. |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The instruction to provide a fallback to FindObjectsOfType contradicts the performance goals stated in line 25. If the NetworkMap is null (e.g., in the main menu), it is highly probable that no racks exist. Consider updating this documentation to recommend a default return of 0 to ensure the API remains O(1) and avoids heap allocations.
💡 What: Replaced the O(N)
FindObjectsOfType<global::Il2Cpp.Rack>()call inGetRackCount()with an O(1) lookup usingIl2Cpp.NetworkMap.instance.GetNumberOfDevices().🎯 Why:
FindObjectsOfTypeis extremely expensive as it iterates through the entire object hierarchy. This optimizes the count retrieval, particularly in late-game stages where the number of objects is high, while maintaining a safe fallback when the game is uninitialized.📊 Impact: Reduces CPU hitches and Garbage Collection pressure by avoiding an expensive global object search. The complexity for getting the rack count is reduced from O(N) to O(1).
🔬 Measurement: Use Unity Profiler to measure the CPU time and GC allocations of
GetRackCount()during gameplay with many Racks in the scene. The time should be nearly unmeasurable now.PR created automatically by Jules for task 13745328081209463155 started by @mleem97