⚡ Bolt: O(1) constant-time lookups for execution registry#180
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the ExecutionRegistry class by replacing O(N) linear list traversals with O(1) dictionary lookups using functools.cached_property. Additionally, it updates the project documentation to include this optimization pattern. The reviewer suggested wrapping the cached dictionaries in MappingProxyType to improve encapsulation and ensure immutability, which aligns with existing patterns in the codebase.
| def _command_map(self) -> dict[str, MirroredCommand]: | ||
| return {cmd.name.lower(): cmd for cmd in reversed(self.commands)} |
There was a problem hiding this comment.
For better encapsulation and to prevent accidental modification of the cached data, it's a good practice to return an immutable mapping. This aligns with the frozen=True nature of the ExecutionRegistry dataclass and is consistent with the pattern used in _get_command_lookup in src/commands.py.
You'll need to add the following imports at the top of the file:
from collections.abc import Mapping
from types import MappingProxyType| def _command_map(self) -> dict[str, MirroredCommand]: | |
| return {cmd.name.lower(): cmd for cmd in reversed(self.commands)} | |
| def _command_map(self) -> Mapping[str, MirroredCommand]: | |
| return MappingProxyType({cmd.name.lower(): cmd for cmd in reversed(self.commands)}) |
| def _tool_map(self) -> dict[str, MirroredTool]: | ||
| return {tool.name.lower(): tool for tool in reversed(self.tools)} |
There was a problem hiding this comment.
Similar to the _command_map, returning an immutable MappingProxyType here would make the implementation more robust and consistent with existing patterns in the codebase (e.g., _get_tool_lookup in src/tools.py).
This change requires the following imports:
from collections.abc import Mapping
from types import MappingProxyType| def _tool_map(self) -> dict[str, MirroredTool]: | |
| return {tool.name.lower(): tool for tool in reversed(self.tools)} | |
| def _tool_map(self) -> Mapping[str, MirroredTool]: | |
| return MappingProxyType({tool.name.lower(): tool for tool in reversed(self.tools)}) |
There was a problem hiding this comment.
Pull request overview
This PR optimizes ExecutionRegistry command/tool name lookups by replacing linear scans over tuples with cached, dictionary-backed maps to achieve O(1) average lookup time during routing.
Changes:
- Added lazily computed
@functools.cached_propertymaps for commands/tools to speed up name-based lookup. - Updated
command()/tool()to use cached dict.get()lookups instead of iterating the tuples. - Added an optimization log entry in
.jules/bolt.mddescribing the change and measured impact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/execution_registry.py | Introduces cached dict-backed lookup maps and switches registry lookups to O(1). |
| .jules/bolt.md | Documents the optimization and its benchmark impact. |
Comments suppressed due to low confidence (1)
src/execution_registry.py:42
- _tool_map has the same mutability/immutability concern as _command_map: it returns a mutable dict cached on a frozen instance. Consider returning an immutable Mapping (e.g.,
MappingProxyType) and aligning duplicate-handling with the repo’s existing lookup builders (if key not in lookup:) for clarity.
@functools.cached_property
def _tool_map(self) -> dict[str, MirroredTool]:
return {tool.name.lower(): tool for tool in reversed(self.tools)}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @functools.cached_property | ||
| def _command_map(self) -> dict[str, MirroredCommand]: | ||
| return {cmd.name.lower(): cmd for cmd in reversed(self.commands)} | ||
|
|
| return {cmd.name.lower(): cmd for cmd in reversed(self.commands)} | ||
|
|
||
| @functools.cached_property | ||
| def _tool_map(self) -> dict[str, MirroredTool]: | ||
| return {tool.name.lower(): tool for tool in reversed(self.tools)} |
| ## 2026-04-29 - Python Dictionary Lookup Optimization for Lists | ||
| **Learning:** O(N) linear scans on static lists inside frequently called functions (e.g., `get_tool`) are a performance hazard. A `lru_cache(maxsize=1)` dictionary lookup wrapped in `MappingProxyType` to prevent modification is a simple and extremely effective optimization technique that reduces complexity from O(N) to O(1) and eliminates repeated loop and string allocations. We must be mindful when mapping non-unique elements. Preserving first-match behavior requires `if key not in lookup:` during the dictionary's initial lazy-loading map phase. | ||
| **Action:** Always favor lazily initialized cached dictionary lookups over list iteration when retrieving elements from static datasets by string keys. | ||
| ## 2024-05-13 - Python Lazy Map Lookups Optimization |
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
@claude code review |
| @functools.cached_property | ||
| def _command_map(self) -> dict[str, MirroredCommand]: | ||
| return {cmd.name.lower(): cmd for cmd in reversed(self.commands)} |
Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/d3feae09-3114-4c34-870d-bdbcaee89121 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Addressed the requested review feedback in commit |
💡 What: Replaced O(N) linear iteration over command and tool tuples with lazily-evaluated O(1) dictionary lookups (
@functools.cached_property) in theExecutionRegistryclass. Preserved first-match-wins logic for duplicate keys by usingreversed().🎯 Why: Prevents repetitive string allocations and list traversals during frequent routing operations when searching for specific tools or commands by name.
📊 Measured Improvement: Reduces average benchmarked time for repeated lookups of unknown strings from ~3.69s down to ~0.06s (~98% speedup).
PR created automatically by Jules for task 11857276324186609613 started by @badMade