perf: O(1) tool lookup in ToolExecutor via lazily-cached name index#449
perf: O(1) tool lookup in ToolExecutor via lazily-cached name index#449clickbrain wants to merge 1 commit intowaybarrios:mainfrom
Conversation
_get_tool_by_name(), _tool_exists(), and _get_server_for_tool() each called manager.get_all_tools() and iterated the full list every time. With many MCP tools registered, every tool call paid an O(N) scan — sometimes multiple scans in a single call path. Add _get_tool_index() which builds a dict keyed by both full_name and short name. The index is rebuilt only when the tool count changes (e.g. after refresh_tools()), so steady-state lookups are O(1). All three lookup helpers now delegate to _get_tool_index().get() or 'in' membership test — both O(1). Co-authored-by: GitHub Copilot <copilot@github.com>
| def _get_tool_index(self) -> dict[str, "MCPTool"]: | ||
| """Return a name→tool index, rebuilding it if the tool list changed.""" | ||
| tools = self.manager.get_all_tools() | ||
| if len(tools) != self._tool_index_size: |
There was a problem hiding this comment.
Is it reasonable to assume that a tool wouldn't be removed simultaneously to a different tool being added?
There was a problem hiding this comment.
I dont think we should have that assumption
Thump604
left a comment
Thump604
left a comment
There was a problem hiding this comment.
I do not think this is merge-ready yet.
The O(1) lookup direction is good, but the cache invalidation rule is too weak. _get_tool_index() only rebuilds when len(manager.get_all_tools()) changes. MCPClient.refresh_tools() and reconnect can replace the discovered tools with the same count, including different names, schemas, or server ownership. In that case the executor keeps returning the old MCPTool objects: _tool_exists() can accept removed tools, _validate_tool_call() can validate against stale schemas, and _get_server_for_tool() can route short-name calls to a stale server.
A safe version needs a stronger invalidation signal, for example a manager/client tool-generation counter bumped after refresh/reconnect/disconnect, or a cheap signature over the current tool identities such as (server_name, name) before reusing the index. It also needs a regression test where the manager's tool list changes from one tool to a different one with the same count and the executor stops accepting the stale tool.
| tools = self.manager.get_all_tools() | ||
| if len(tools) != self._tool_index_size: | ||
| idx: dict[str, "MCPTool"] = {} | ||
| for t in tools: |
There was a problem hiding this comment.
I think this for loop adds some latency, we could rewrite it more efficient.
4 participants
Problem
_get_tool_by_name(),_tool_exists(), and_get_server_for_tool()each callmanager.get_all_tools()and iterate the full list on every invocation. With many MCP tools registered, each tool call pays an O(N) scan — sometimes multiple times per call path.Fix
Add
_get_tool_index()which builds adict[str, MCPTool]indexed by bothfull_nameand shortname. The index is rebuilt only when the tool count changes (e.g. afterrefresh_tools()), so steady-state lookups are O(1).All three lookup helpers now delegate to
_get_tool_index().get()orinmembership — both O(1).