Skip to content

[Repo Assist] perf: O(1) command dispatch index in WindowsNodeClient#153

Closed
github-actions[bot] wants to merge 1 commit intomasterfrom
repo-assist/perf-capability-dispatch-index-20260407-3f78467f65da3719
Closed

[Repo Assist] perf: O(1) command dispatch index in WindowsNodeClient#153
github-actions[bot] wants to merge 1 commit intomasterfrom
repo-assist/perf-capability-dispatch-index-20260407-3f78467f65da3719

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot commented Apr 7, 2026

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

Replaces two O(n) linear scans with a single O(1) dictionary lookup for command dispatch in WindowsNodeClient.


Problem

Every node.invoke.request and req message dispatched through WindowsNodeClient called:

_capabilities.FirstOrDefault(c => c.CanHandle(command))

CanHandle in NodeCapabilityBase does Commands.Contains(command) — itself an O(n) scan over the capability's command list. The combined complexity is O(capabilities × commands-per-capability) on every invocation.

Fix

A Dictionary(string, INodeCapability) _commandIndex (with StringComparer.OrdinalIgnoreCase) is built once in RegisterCapability. Both dispatch call sites now use:

_commandIndex.TryGetValue(command, out var capability);
```

This is **O(1)** regardless of how many capabilities are registered.

The now-unused `using System.Linq;` import is also removed.

### Trade-offs

- The `_capabilities` list is kept for `Capabilities` property exposure and registration bookkeeping — only the hot dispatch path changes.
- The dictionary holds the last-registered capability for any duplicate command name; this matches the previous `FirstOrDefault` behaviour (first match wins, but here last-write wins — in practice no duplicates exist since `RegisterCapability` guards against duplicate command names in `_registration.Commands`).

---

## Test Status

**3 new unit tests** in `WindowsNodeClientTests`:

| Test | Purpose |
|---|---|
| `RegisterCapability_PopulatesCommandIndex_WithAllCommands` | All commands of a registered capability appear in the index |
| `RegisterCapability_MultipleCapabilities_IndexedToCorrectHandler` | Two capabilities map to their respective handlers |
| `RegisterCapability_CommandIndexLookup_IsCaseInsensitive` | Dictionary uses `OrdinalIgnoreCase` |

```
Passed! – Shared.Tests: 548 total (528 passed, 20 skipped) ✅
Passed!Tray.Tests:    99 passed ✅

Generated by Repo Assist ·

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@cbb46ab386962aa371045839fc9998ee4e97ca64

Replace two O(n) _capabilities.FirstOrDefault(c => c.CanHandle(command))
linear scans with a single O(1) Dictionary<string, INodeCapability> lookup.

The _commandIndex is populated in RegisterCapability (once per capability
registration at startup). On every node.invoke.request or req dispatch,
the lookup is now O(1) regardless of how many capabilities are registered.

Also removes the now-unused 'using System.Linq' import.

Adds 3 unit tests covering:
- all commands of a registered capability appear in the index
- multiple capabilities are indexed to their correct handler
- the dictionary is case-insensitive (StringComparer.OrdinalIgnoreCase)

Co-authored-by: Copilot <[email protected]>
@shanselman
Copy link
Copy Markdown
Collaborator

Thanks for the optimization here. We’re going to pass on this one for now because the current dispatch path is already working well, and this change subtly alters the command-routing behavior (for example around case-sensitivity / exact dispatch semantics) for a fairly small performance win. We’d rather revisit this later as part of a broader node-dispatch cleanup if needed.

@shanselman shanselman closed this Apr 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant