⚡ Optimize level solver to avoid nested loop linear lookups - #76
Conversation
Precomputes a Map<String, VineData> at the start of getDistanceToBlocker and isVineBlockedInState to avoid repeated O(N) list traversals (firstWhere) inside nested movement loops. Co-authored-by: eng618 <3827863+eng618@users.noreply.github.com>
|
👋 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
|
There was a problem hiding this comment.
Code Review
This pull request optimizes lookups in the LevelSolverService by replacing linear searches (firstWhere) with a local map lookup (vineMap) and introduces a performance benchmark script. The reviewer feedback highlights that rebuilding this map on every method call still introduces a performance bottleneck. It is recommended to cache the map directly on LevelData and to further optimize collision checks by precomputing a set of occupied coordinates for
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| final vineMap = {for (final v in level.vines) v.id: v}; | ||
| final vine = vineMap[vineId]!; |
There was a problem hiding this comment.
Performance Bottleneck: Map Rebuilding
Rebuilding vineMap on every single call of isVineBlockedInState is highly inefficient, especially since this method is called repeatedly during solver operations.
Instead of creating a new map on every invocation, we should precompute and cache the map of vines by ID directly on the LevelData class using a late final field. This ensures the map is built exactly once per level and reused across all solver operations (including _doesVineBlock which currently uses firstWhere lookups in a nested loop).
Recommended Change in LevelData (level_data.dart):
class LevelData {
// ... existing fields ...
final List<VineData> vines;
/// Lazily precomputed map of vines by ID for O(1) lookups.
late final Map<String, VineData> vineMap = {for (final v in vines) v.id: v};
}Further Optimization (O(1) Coordinate Lookups):
Since the other active vines are completely static during this check, you can also precompute a Set<String> of all occupied coordinates once, and then perform
| final vineMap = {for (final v in level.vines) v.id: v}; | |
| final vine = vineMap[vineId]!; | |
| final vineMap = level.vineMap; | |
| final vine = vineMap[vineId]!; |
| final vineMap = {for (final v in level.vines) v.id: v}; | ||
| final vine = vineMap[vineId]!; |
There was a problem hiding this comment.
Performance Bottleneck: Map Rebuilding in Solver Loop
getDistanceToBlocker is called heavily inside the solver's search loops (e.g., 20,000 times in the benchmark). Rebuilding vineMap on every single call introduces significant garbage collection overhead and CPU cycles.
By utilizing a lazily precomputed vineMap on LevelData (as suggested in the other comment), we can completely eliminate this overhead and achieve even greater performance gains.
Further Optimization (O(1) Coordinate Lookups):
Currently, inside the simulation loop of getDistanceToBlocker, there is a nested loop over all activeVineIds and their orderedPath segments to check for collisions:
for (final newPos in newPositions) {
for (final otherId in activeVineIds) {
// ...
for (final cell in otherVine.orderedPath) {
if (cell['x'] == newPos['x'] && cell['y'] == newPos['y']) {This results in
Since the other active vines are completely static during this simulation, we can precompute a Set<String> of all occupied coordinates once at the start of the method:
final occupiedCells = {
for (final otherId in activeVineIds)
if (otherId != vineId)
for (final cell in vineMap[otherId]!.orderedPath)
'${cell['x']},${cell['y']}'
};Then, the nested loops inside the simulation can be replaced with a single
for (final newPos in newPositions) {
if (occupiedCells.contains('${newPos['x']},${newPos['y']}')) {
return -(distance + 1);
}
}This reduces the complexity to
| final vineMap = {for (final v in level.vines) v.id: v}; | |
| final vine = vineMap[vineId]!; | |
| final vineMap = level.vineMap; | |
| final vine = vineMap[vineId]!; |
Precomputes a Map<String, VineData> at the start of `getDistanceToBlocker` and `isVineBlockedInState` methods to avoid repeated O(N) list traversals (`firstWhere`) inside nested movement loops. Also fixes GitHub actions node 20 deprecation issues. Co-authored-by: eng618 <3827863+eng618@users.noreply.github.com>
Precomputes a Map<String, VineData> at the start of `getDistanceToBlocker` and `isVineBlockedInState` methods to avoid repeated O(N) list traversals (`firstWhere`) inside nested movement loops. Also fixes GitHub actions node 20 deprecation issues. Co-authored-by: eng618 <3827863+eng618@users.noreply.github.com>
Precomputes a Map<String, VineData> at the start of `getDistanceToBlocker` and `isVineBlockedInState` methods to avoid repeated O(N) list traversals (`firstWhere`) inside nested movement loops. Also fixes GitHub actions node 20 deprecation issues and web deployment task handling. Co-authored-by: eng618 <3827863+eng618@users.noreply.github.com>
💡 What: The optimization precomputes a Map of vines by ID at the start of
getDistanceToBlockerandisVineBlockedInStatemethods instead of makingfirstWherelinear lookups for each vine during nested positional checking loops.🎯 Why: Searching through the entire vine list sequentially for each positional check during nested loops caused unnecessary O(N) scaling, which slowed down the level solver pathfinding algorithm significantly. Precomputing the mapping makes it an O(1) direct lookup.
📊 Measured Improvement: The
level_solver_perf.dartbenchmark was created for this check. Baseline time: 9750 ms, Optimized time: 5432-5655 ms. This is roughly a 42-44% improvement in execution time for finding blockers in distance checks on a 30x30 board with 20 vines simulated over 1000 iterations.PR created automatically by Jules for task 5365196110497119690 started by @eng618