Reduce unresolved graph edges: dataflow placeholder binding + lone-method guard#258
Merged
Conversation
arg_of / value_flow edges are emitted with an unresolved:: placeholder on the callee side. The call resolver already binds `calls` edges to those callees, but it scopes the candidate lookup to the edge's From node's repo, and a dataflow edge's From is an unresolved:: argument placeholder with no node — so the lookup is scoped to the empty repo, matches nothing in a multi-repo graph, and the callee never lifts. materializeDataflowParams then skips the edge because its target is still an unresolved:: stub. The result was roughly half of all arg_of edges left dangling placeholder->placeholder even when the callee was defined in the same file, both bloating the resolver's pending set and breaking flow_between / taint across those hops. Add a cheap post-resolve pass that binds arg_of / value_flow bare-name callees to the sole same-file function or method via a prebuilt file->name index (O(1) per edge). It matches by FilePath equality, so it needs no repo-prefix logic and behaves identically in bare and prefixed graphs, and it runs in the attribution tail rather than the hot per-edge loop. Let builtin attribution fire for dataflow edges into Go builtins by falling back to the .go file extension when the From placeholder can't be classified. Also add per-sub-phase resolve timing and a pending-edge shape breakdown to the resolver logs, to make cold-index resolve cost attributable.
IndexRepo gated repo-prefix stamping and eviction on IsMultiRepo(), so a single tracked repo was indexed bare while the cold-index path always prefixed. The two paths produced different id and path shapes for the same repo, and adding a second repo turned the first from bare into prefixed — a lossy whole-graph rewrite. Always stamp the prefix so a repo is prefixed from its first index and adding another is purely additive. Also make RunGlobalGraphPasses log each sub-pass unconditionally with its elapsed time. They were logged only when a pass emitted edges, so a slow low-yield pass left no breadcrumb — the multi-minute silent span after resolve on a cold index.
Extend the dataflow-callee bind pass beyond same-file:
- a bare-name callee with no same-file definition now falls back to the
sole same-package function of that name (Go package function names are
unique, so the same-dir match is unambiguous), reusing r.dirIndex so it
stays a lookup, not a scan, and works the same on the incremental path.
- a `*.method` callee is lifted onto the method the call resolver already
bound for a calls / references edge at the same call site (file+line),
matched by method name — reusing the receiver-type resolution done there
instead of re-deriving it.
Together with the same-file bind this takes arg_of-unresolved on a Go repo
from -83% to -90% with no change to resolve time. What remains is method-call
args whose underlying call the resolver itself could not bind, and the
handful of cross-package / undefined names — neither of which a dataflow
bind can resolve.
… guard A method call carries its receiver, so it needs no import of the method's package — but the cross-package guard reverts a name-only member-call bind whose package the caller's file doesn't import, treating it like a free function. That reverted every call to a method whose only in-repo definition lives in a package the caller reaches indirectly (a receiver returned from another package, an embedded/inherited method): the import closure never names the declaring package, so the guard dropped an otherwise unambiguous resolution. The lone-member-definition exception that already protects Java is language-agnostic in substance, so generalize it to Go: when the member call's method name has exactly one in-repo method definition, keep the bind (and lift it to ast_inferred in resolveMethodCall so it isn't a suppressed text_matched guess). Restricted to statically-typed languages (java, go) — TS/Python duck typing makes a same-name coincidence likelier, so their guard revert stays — to member calls only (a bare Go free-function call to an un-imported package still reverts), and gated on the receiver, when its type is known, naming an in-repo type so an external-typed receiver still reverts. On a Go repo this resolves ~97% of the previously-reverted lone-method calls (unique-name member calls left unresolved: 3,366 -> 85) with no change to resolve time.
… languages The lone in-repo method-definition exception that keeps an otherwise-reverted member-call bind is unambiguous in any statically-typed language, not just Java and Go. Extend it to Rust, C#, Kotlin, and Scala (member calls only; the receiver-type-in-repo gate and the n==1 uniqueness guard are unchanged). Duck- typed languages (TS / JS / Python) stay excluded — a lone same-name coincidence is likelier there, so their guard revert remains load-bearing. On a Rust repo this resolves the previously-reverted lone-method member calls (~277 on reader-rust), binds verified correct (FileCache.put, AiModelConfig.resolve), no change to resolve time.
The chunked/full ResolveAll apply loop and the cross-package guard call edgeStillLive once per applied edge to confirm an interleaving edit hasn't evicted the row. On a sqlite-backed daemon that ran GetOutEdges — a query that materialises and gob-decodes every one of the From node's out-edges — only to compare five fields and discard the rest. On a cold/full pass over a large graph that was ~40% of resolve CPU (and the bulk of its GC churn) for a plain yes/no existence question. Add Store.EdgeExists: one point lookup on the edges UNIQUE (from,to,kind,file_path,line) index — no row decode, no Meta blob, no per-edge allocation. edgeStillLive uses it through an optional edgeExister interface and falls back to the out-edge scan for the in-memory graph (already cheap: no disk, no gob). Same predicate, same result: the value-match branch is exactly what fired on sqlite before (the pointer test never matches a freshly-decoded row), so the applied/skipped set — and thus every resolved edge — is byte-identical. Measured on a 4-repo cold index: ResolveAll 552s to 159s, the cross-package guard 283s to 11s, the compute+apply loop 241s to 34s; reindex_batch unchanged at 557,072.
GORTEX_RESOLVE_CPUPROFILE=<path> captures a CPU profile of the first full (unscoped) ResolveAll and writes it on completion. Env-gated and guarded by a one-shot flag so it never touches steady-state resolution; complements the per-sub-phase timing when a coarse breakdown isn't enough to localise a regression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A data-driven pass over the unresolved edges in a Go repo's graph found the dominant class isn't stdlib/external noise — it's the dataflow / CPG layer. Store-wide, ~46% of all
arg_ofedges point atunresolved::placeholders,and the large majority of those have both endpoints unresolved — dangling placeholder→placeholder hops such as
arg_of: unresolved::contractFields → unresolved::newGCX, where both are real functions defined in the same file.These edges do double damage: they bloat the resolver's pending set (every one is re-examined on every full pass), and they break
flow_between/taintacross the hop because neither end points at a real node.Root cause:
resolveEdgeonly ever rewritese.To, and it lifts a bare-name callee via the From node's repo prefix. A dataflow edge'sFromis anunresolved::argument placeholder with no node, so the candidate lookup is scoped to the empty repo, matches nothing in a multi-repo graph, and the callee never lifts.materializeDataflowParamsthen skips the edge because its target is still anunresolved::stub. Go-builtin attribution fails the same way — it gates on the From node's language.Fix
bindDataflowCalleeRefs(newinternal/resolver/dataflow_callee_bind.go): a cheap post-resolve pass that bindsarg_of/value_flowbare-name callees to the sole same-file function/method via a prebuiltfile → nameindex (O(1) per edge). It matches byFilePathequality, so it needs no repo-prefix logic and behaves identically in bare and prefixed graphs, and it runs in the attribution tail rather than the hot per-edge loop..gofile extension when the From placeholder can't be classified, soappend/make/lenarguments attribute to their builtin node.IndexReponow always stamps the repo prefix, matching the cold-index path — a repo is prefixed from its first index and adding another is purely additive, instead of a lossy bare→prefixed rewrite.Measured (Go repo, cold index)
arg_of-unresolvedarg_of+value_flow)arg_ofinto builtins remainingcompute_loopRemaining (out of scope for this change)
arg_ofto*.method/ qualified targets — need the receiver-type method resolver, a separate change.Note on the prefixing change
New and existing prefixed stores are consistent; an existing bare single-repo store would want a one-time cold re-index (the prefix-scoped eviction won't clear bare rows). The daemon's cold-index path already prefixes unconditionally, so this closes a latent inconsistency rather than migrating live data.
Follow-up: keep lone in-repo method binds against the cross-package guard
While tracing the remaining
*.methoddataflow residual, the biggest cause turned out to be upstream: the method calls themselves don't resolve, so there's nothing for the dataflow join to point at. A large, cleanly-fixable slice of that is a guard bug: a method call carries its receiver and needs no import of the method's package, but the cross-package guard reverted a lone-method bind whose package the caller doesn't directly import (a receiver returned from elsewhere, an embedded/inherited method). The lone-member-definition exception that already protects Java is generalized to Go (member calls only; a bare free-function call to an un-imported package still reverts; gated on a known receiver type naming an in-repo type).Result on a Go repo: unique-name member calls left unresolved 3,366 → 85 (−97%), no change to resolve time. What remains is genuinely upstream — ~9k calls to methods with no in-repo definition (interface / embedded / external) and ~1.4k ambiguous names that need real receiver-type inference.
Follow-up: cut resolve wall-time with an indexed edge-liveness lookup
Profiling the cold/full resolve pass found that ~40% of resolve CPU — and most
of its GC churn — went to a single yes/no question. The chunked apply loop and
the cross-package guard call
edgeStillLiveonce per applied edge to confirm aninterleaving edit hasn't evicted the row; on a sqlite-backed daemon that ran
GetOutEdges, which materialises and gob-decodes every one of the From node'sout-edges just to compare five fields.
Store.EdgeExistsanswers it as a single point lookup on the edgesUNIQUE (from,to,kind,file_path,line)index — no row decode, no Meta blob, no per-edgeallocation.
edgeStillLiveuses it via an optional interface and keeps theout-edge scan as a fallback for the in-memory graph. Same predicate, same result
(the value-match branch is exactly what fired on sqlite; the pointer test never
matched a freshly-decoded row), so every resolved edge is byte-identical —
reindex_batchis 557,072 before and after.ResolveAlltotaledgeStillLiveshare of resolve CPUreindex_batch)