Skip to content

fix(graph): impact reads a node's edges in one unlimited query again (#3248) - #3249

Open
inth3shadows wants to merge 1 commit into
colbymchenry:mainfrom
inth3shadows:fix/3248-impact-incoming-query
Open

inth3shadows wants to merge 1 commit into
colbymchenry:mainfrom
inth3shadows:fix/3248-impact-incoming-query

Conversation

@inth3shadows

Copy link
Copy Markdown
Contributor

Fixes #3248

Impact traversal reads each visited node's dependents with one unlimited query again. #3125 turned that read into kind IN (<every kind but contains>) … LIMIT ?, and both parts slowed down a query the walk runs once per visited node. getImpactRadius took 2–3x as long for the same subgraph. The node and edge budgets from #3125 still apply, by slicing what the read returns.

Changes

  • src/db/queries.ts: new getIncomingEdgesExceptContains(targetId), a cached statement SELECT * FROM edges WHERE target = ? AND kind != 'contains' ORDER BY kind, source, line, col. It's one index search on target, with the same order as getIncomingEdges. kind != 'contains' is the predicate the file already uses for this rule elsewhere.
  • src/graph/traversal.ts, in getImpactRecursive:
    • incoming edges come from the new read with no LIMIT, and the slice applies the edge budget, so truncated still means the budget dropped a dependent;
    • a container's members come from getOutgoingEdges(nodeId, ['contains']), also without a LIMIT;
    • IMPACT_INCOMING_KINDS and the EDGE_KINDS import are removed, since nothing else uses them.
  • __tests__/graph.test.ts:
    • real SQLite: for every node of the fixture, the new read equals getIncomingEdges minus contains, in the same order;
    • fake query layer: no read in the walk passes a LIMIT (incoming, container, or an extra argument to the new read), and contains never reaches the result, takes a budget slot or marks the result truncated, while a real second dependent over budget does set truncated;
    • tGraph's fake gains the new method.

Known limits

These came up in review and are left as they are:

Validation

  • Output equals main:
    • 20,000 random graphs: up to 13 nodes and 59 edges of all 13 kinds, with cycles and self-loops, depths 0–4, maxEdges 0–11, maxNodes 1–8. That gives 0 differences in nodes, edges with their order, and truncated, across 8,735 truncated runs.
    • 500 traversals of codegraph's own index: the 25 busiest targets and 25 classes or interfaces, at depths 1 and 3, under five budgets. 0 differences.
  • New fake-layer test:
    • on main's traversal: AssertionError: expected false to be true, 1 failed;
    • with only the container read's LIMIT restored: it also fails;
    • with this change: it passes.
  • tsc --noEmit exits 0. Full suite: Test Files 1 failed | 866 passed | 19 skipped (886), Tests 1 failed | 10521 passed. The one failure is the viewer test described at the end (expected 33 to be greater than 300), which fails the same way on main.
  • Timings. codegraph's own source indexed by each build, best of 5. The range covers 3 rounds with the builds interleaved. Linux (WSL2, 12 cores), Node 22.23.2.
build getImpactRadius init / getNode /api/node init / close / getNode
b6ea6179 (before #3125) 22–23 / 32–33 ms 87–88 / 68–72 / 90–93 ms
f72c9d2e (main) 62–65 / 70–99 ms 126–135 / 94–96 / 131–134 ms

What each part of the read costs. This is getImpactRadius for init, 5 interleaved rounds on a loaded machine (load average 8–15), so compare the rows with each other:

read in the walk init
main: kind list + LIMIT 99–122 ms
LIMIT kept, kind != 'contains' 87–94 ms
kind list kept, no LIMIT 58–70 ms
unfiltered, contains dropped in JS 45–52 ms
this PR: kind != 'contains', no LIMIT 37–46 ms

Impact on large classes, which also runs the container branch, returned the same subgraph in every build. CodeGraph (2,417 nodes, 17,449 edges) took 710–900 ms on main and 423–624 ms with this change (load average 15–17). The container change on its own was within noise there, since a walk reads few containers. It's in this PR because it is the same pattern in the same function.

The viewer test "the engine's own busiest symbol" still targets LRUCache.get, whose fan-in is now 33, so it fails fanIn > 300 in a checkout that has indexed itself, on main and here alike. Pointing it at CodeGraph.init would catch this slowdown: it fails on main at 176 and 145 ms. But with the fix it runs at 83–95 ms against a 100 ms limit, too thin a margin to add here. #2646 touches that test; I left a comment there.

…olbymchenry#3248)

colbymchenry#3125 gave impact traversal a node and edge budget, and fetched each visited
node's incoming edges as `kind IN (<every kind but contains>) ... LIMIT ?`.
Both halves slowed the read the walk makes once per visited node: the kind
list searches the target index once per kind, and the LIMIT under the
read's ORDER BY made it slower still. getImpactRadius took 2-3x as long for
the same subgraph.

The walk now reads `WHERE target = ? AND kind != 'contains'` with no LIMIT,
through a cached statement, and the slice applies the budget as before. A
container's members are read without a LIMIT too, for the same reason. A
contains edge still never takes a budget slot or marks the result
truncated, and the output is unchanged: the same nodes, edges, order and
truncation as main on 20,000 random graphs and 500 traversals of
codegraph's own index.

Fixes colbymchenry#3248

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Impact traversal is 2–3x slower since #3125: incoming edges are fetched with a 12-kind IN list

1 participant