lsp: definition-confirm sweep, drop the heavy request classes for C# servers - #634
Conversation
Per-spec MaxParallel is hardcoded in the registry, so an operator whose server multiplexes better than the conservative default has no way to try a higher cap without editing source. Env wins over the spec value at spawn, same shape as GORTEX_LSP_SWEEP; unusable values fall through.
semantic.lsp_max_parallel overrides every spec's registry default when positive, threaded router-to-provider the same way lsp_sweep is; the GORTEX_LSP_MAX_PARALLEL env override keeps the last word, spec default as fallback - resolveMaxParallel mirrors resolveSweepMode's precedence.
csharp-ls schedules read-only requests concurrently but treats every didOpen / didClose as an exclusive write that drains all in-flight reads, so an enrichment pass that interleaves opens serializes to single-request throughput regardless of maxParallel. Roslyn answers hover / references / call hierarchy for never-opened files, so a spec that sets NoDidOpen now runs the whole pass without the document lifecycle: the shared docSession degrades to a bounded content cache and sends no notifications. GORTEX_LSP_OPEN_DOCS overrides both ways.
Same precedence shape as lsp_sweep: env > config > spec > default. Empty (the default) lets each spec's NoDidOpen decide; "on" forces the lifecycle for every server, "off" skips it everywhere.
closeAll's didClose is a fire-and-forget notification; asserting the pairing immediately races the pipe.
…ferences csharp-ls leaks per request on Roslyn's FindReferences machinery — ~10MB and a set of OS handles per textDocument/references round trip, ~0.7MB per callHierarchy/incomingCalls — and nothing is released short of a server restart, so a barrier-free enrichment pass collapses the process tens of GB deep before it completes. Position requests are clean (hover and definition both measure ~1KB/request, flat over hundreds of thousands of requests). ServerSpec.NoHeavyRequests (set for csharp-ls/omnisharp) now disables both request classes. Ambiguous edges route straight to the existing definition pass at their call sites, which reaches the same confirm / rebind verdicts from the cheap direction — ask the site what it binds to instead of asking the callee who references it. Two verdict shapes the references path used to paper over are handled explicitly: - definition on `new T(...)` answers the constructor's line; a type named for the target whose span contains that line confirms the instantiates edge (findEnclosingTypeNamed). - definition on a dispatched call answers the DECLARED member. When the stored edge targets one of its concrete impls, the declared- member edge is added at LSP grade and the impl edge — a devirtualization inference the compiler cannot vouch for — is left untouched at its heuristic tier; impl reachability flows through the interface-dispatch synthesis. An unrelated misbind still rebinds as before (implementsDeclaredMember separates the two). Measured on a 120-edge C# probe corpus: definition reproduces every edge the references path stored — 108 exact, 4 ctor-shape, 8 interface-shape — at ~1,100 requests/s where references sustained 2-10/s with an unbounded ramp.
The NoHeavyRequests spec flag guards stock csharp-ls builds, but the leak it works around has a known upstream fix — an operator running a patched server build wants references / incomingCalls back without rebuilding gortex. GORTEX_LSP_HEAVY wins over the spec flag both ways, mirroring GORTEX_LSP_OPEN_DOCS: "on" restores the heavy legs for an opted-out spec, "off" disables them for every server, anything else falls through to the spec.
# Conflicts: # internal/config/config.go # internal/semantic/config.go # internal/semantic/lsp/router.go # internal/serverstack/shared_server.go
The definition-rebind loop ran serially — a didOpen-era constraint that kept call-site document opens from overlapping goroutines. With the lifecycle gone for NoDidOpen servers, and the heavy opt-out routing the whole confirm load through this pass, the serialization became the sweep's longest pole: a large C# solution answers tens of thousands of definitions one at a time while the server sits mostly idle. Group the sorted targets by call-site file — one session acquire and one site cache per group, both group-local by construction — and fan the groups out across maxParallel, the same shape the reference-confirm sweep already uses. The verdict switch (which reads the view's edge indexes that staged mutations update) moves entirely under rmu; the definition round trip stays outside it.
241k requests took the same 26 minutes at maxParallel 6 and 12, so the pole is not slot starvation — but one aggregate duration cannot say which pass owns the minutes. Stamp each phase boundary (setup, impls, confirm, definitions, refs-add, sweep) and emit phase_*_ms alongside the request counters.
Under the heavy opt-out the definition pass carries the whole confirm load, but nothing in it ever observed the failure-streak breaker — a server that errors every request would grind through every sited target at one timeout each instead of aborting after the streak limit. Observe each definition round trip; cache hits, identifier misses, and answered-but-empty responses never touch the breaker.
Four findings from a fresh-eye review of the branch: - The definition pass fed no usefulYield, so the productivity checkpoint read a warm-graph no-heavy pass (interface adds ~0, def pass the only confirm source until hover) as zero-yield and cancelled it at the first tick after request volume flowed. Feed it from all three productive arms; pinned by a checkpoint-survival test that reproduces the cut. - ConfirmSymbolRefs still issued callHierarchy/incomingCalls for NoHeavyRequests servers — the on-demand find_usages path would accumulate the FindReferences leak one query at a time in a long-lived daemon. Gated. - The no-heavy fallback was built from confirmGroups, inheriting referent-side drops (no position, unserved referent file) that exist only because findReferences opens the referent's file. Build it from the raw sited-target list; a misbound edge with an unusable stored target is exactly what the rebind arm is for. - docs/lsp.md overstated the config knob's reach (router-spawned providers only).
Main landed the definition-fallback yield (92a09c9) and the rebound ledger split (4a9ed41) independently — the same checkpoint gap this branch fixed in its review round, plus rebinds counted as EdgesRebound instead of EdgesConfirmed. Resolution keeps the parallel definition pass from this branch and adopts main's counter semantics in its rebind arm; main's rebind-ledger test passes against the parallel pass unchanged.
|
Addendum with fresh numbers, since the picture improved again since I wrote the PR body. csharp-ls merged the ProxyGenerator lock fix (razzmatazz/csharp-language-server#410) and I have two more PRs in flight there: outgoingCalls (razzmatazz/csharp-language-server#412) and a document-lookup fix that stops scanning all workspace documents on every request (razzmatazz/csharp-language-server#414). With the document-lookup fix applied the defconfirm numbers move a lot:
Which changes the shape of the Follow-ups section, but only partly. After this stack plus the csharp-ls fixes, LSP is not the cold-start bottleneck anymore: the resolver is, at roughly 45% of the cold wall on my measurement, and that is probably the interesting next conversation for first-run cost. But I still think the background lane / sidecar idea stands. Everything fast here is the light path: heavy mode (references + incomingCalls, the extra ~24k confirms and ~14.5k added edges) still costs several times the wall and is exactly the kind of work a background lane should chew on after the fast pass. And there is territory we have not measured at all, generated code being the big one, my repo has none of it and the perf tax there is unknown. So: fast path much improved, deep path and the unknowns still argue for the sidecar conversation. Coverage note so nobody trips on it: my earlier runs reported 74.5% and these report 67.7%. That delta is #615 doing its job (the sweep gate skipping hover work in bare-data-type files), not a regression. Edge recall is identical across all these runs. |
There was a problem hiding this comment.
Impressive change, and the write-up earns its length — recording the wrong turns (parallelize → −7%) is more useful than the diff. Two things to fix before merge, one stale comment, some polish.
Checked and correct — no need to re-argue these
- Gating is complete. All four heavy-request sites are covered: confirm sweep (
provider.go:1189),incomingCallsin the sweep (:1727),ConfirmSymbolRefs(:2987, behind the early return at:2951),referencesAddPass(only caller:1410). No path left open. - The parallel definition pass is race-correct. Staged mutations, the view edge-index updates they trigger, the verdict switch's edge reads, and the
result.Edges*counters all run underrmu. The out-of-lock reads (view.nodesByID,findDeclarationNode,findEnclosingTypeNamed) touch onlynodesByID/nodesByFile, which nothing mutates during this pass. Groups are unique per call-site file, so no two goroutines open the same document.go test -race ./internal/semantic/lsp/...is clean. docSessionwithsendOpens=falsestill evicts —delete(cd.open, evPath)is outside the gate, so the content cache stays bounded.- Build,
golangci-lint, and./internal/semantic/... ./internal/config/... ./internal/serverstack/...all pass.
1. Default-path change to edge topology, untested there
The new declaredDispatchMember && implementsDeclaredMember arm sits in the shared definition-rebind pass, so it runs for every server. You flag this in the body. I measured what it does, running the same scenario on both trees with the heavy legs on:
- main: the impl edge is rebound —
To→IScale.Weigh, originlsp_resolved, confidence 1.0. - this PR: the impl edge is untouched (
DrumScale.Weigh,ast_inferred, 0.7) and a newlsp_resolvededge toIScale.Weighis added.
So one call site now carries two call edges where it carried one.
Every behavior test for this arm sets p.noHeavyRequests = true. The one heavy-default test only asserts that references are still issued. And every number in the PR is C# / opt-out.
I think the new behavior is more correct — definition answering the declared member does not disprove the devirtualization guess. The gap is coverage and blast radius, not the choice.
What I would want measured: the interface-dispatch synthesizer fans out from confirmed edges, and your own table shows 162.6k downstream INFERRED edges from the extra confirms. Added declared-member edges may amplify graph size on gopls / tsserver / pyright repos, which no run here covered.
Ask: one test on the default path — drop p.noHeavyRequests = true and answer references empty; it reproduces cleanly. Plus a sentence on the expected fan-out for non-C# servers.
2. The headline feature is documented nowhere
GORTEX_LSP_HEAVY and NoHeavyRequests appear in zero files under docs/. Both secondary knobs got docs — GORTEX_LSP_OPEN_DOCS with the scheduler rationale, semantic.lsp_max_parallel — but the mode that changes what C# enrichment produces did not.
The warning matters most. Every released csharp-ls (≤0.26) leaks ~10MB per references / incomingCalls request and passes 30GB on a full pass, so GORTEX_LSP_HEAVY=on is only safe on a build carrying upstream #410. That warning currently lives only in the PR description, which nobody reads at runtime.
Also undocumented: ConfirmSymbolRefs returns early for these servers, so C# find_usages / get_callers lose on-demand LSP confirmation.
Ask: a docs/lsp.md section covering the mode, the override, the OOM warning, and the find_usages consequence.
3. A comment that now contradicts the code
provider.go:1104-1108 still says:
The definition-rebind fallback opens arbitrary call-site files, so it runs serially afterward over the targets the sweep left unconfirmed, keeping document open/close from overlapping across goroutines.
That serial loop is exactly what this PR replaced with a maxParallel fan-out.
Minor
confirmGroups(provider.go:1109) is computed unconditionally but unused on the noHeavy path — move it into theelse.resolveNoHeavyRequestsreusesnormalizeOpenDocs, a helper named for a different concern.normalizeOnOffwould read better.- Knob asymmetry: open-docs and max-parallel are config-settable, heavy mode is env-only. Deliberate per the body, but inconsistent for an operator.
docSession.evictionsstops counting whensendOpensis false, so content-cache churn becomes invisible.- The PR bundles three separable features. That is what makes the default-path change easy to miss.
Fix 1 and 2 and I am happy to merge. The gating and concurrency work I would take as-is.
The add-not-rebind verdict for dispatched call sites lives in the shared definition-rebind pass, so it changes edge topology for every server, not just the heavy-opt-out ones — but every behavior test for the arm set noHeavyRequests. Cover the default path: references sweep runs and answers empty, definition answers the declared member, and the site must keep its devirtualization guess while gaining the compiler-proven edge (the old behavior rebound the impl edge instead). The dispatched-call fixture moves into a shared helper for both tests.
The mode that changes what C# enrichment produces was the one knob with no docs presence. Add a section to the enrichment cost model beside its open-docs sibling: what NoHeavyRequests skips (references confirm, incomingCalls, references-add, on-demand find_usages/get_callers confirmation), what replaces it (definition-at-call-site confirms, the dispatch synthesizer), the GORTEX_LSP_HEAVY override and why it is env-only, and the warning that heavy=on is only safe on a csharp-ls build carrying the upstream FindReferences leak fix. Also bring the definition-rebind phase description up to date with the declared-member verdict: add-not-rebind for devirtualization guesses.
The definition-rebind fallback fans out across maxParallel grouped by call-site file — the serial loop the comment described is what this branch replaced.
…alizer confirmGroups was computed unconditionally but the noHeavy path never reads it (that path deliberately builds from the raw target list); move the grouping into the sweep branch. And normalizeOpenDocs stopped being open-docs-specific the moment the heavy override borrowed it — rename to normalizeOnOff for the shared on/off vocabulary.
A sendOpens=false session still evicts to keep the cache bounded, but the counter sat inside the lifecycle gate, so doc_evictions read zero exactly where cache churn is the only signal left. Count the eviction where it happens; didOpens / peak stay honestly zero for a lifecycle-off pass.
|
All addressed, in five commits on the branch: 1 — default path. On expected fan-out for non-C# servers: the arm only fires for a site the references sweep left unconfirmed, where definition answers a declared dispatch member whose stored target is one of its concrete impls. Worst case is one added declared-member call edge per such site (guarded by 2 — docs. New section in 3 — stale comment. Fixed. Minors: |
2 participants
The chase (context first, how we got here)
This PR looks like "skip two request classes". That is the bottom of a
rabbit hole, not what I set out to build. The path is probably more useful
than the diff, so here it is.
When the C# LSP lane first ran end-to-end on my production C# codebase it took
99 minutes for 15.9% coverage. And before that could even improve, the
server itself was disqualifying: every references/incomingCalls request
leaked, so a full pass grew csharp-ls past 30GB and it never recovered. I
tracked that down to a per-request Castle ProxyGenerator in csharp-ls and
fixed it upstream (csharp-ls #410, merged). That fix is what made the heavy
request classes measurable at all.
With the leak fixed and the didOpen barriers removed, the pass completed at
26 minutes / 67%. At that point every signal said concurrency: ~240k
requests trickling at ~150/s against a server I had benchmarked in the
thousands per second. So I did the obvious things. Parallelized the serial
definition pass, doubled max_parallel from 6 to 12, expected to roughly
halve the wall. Result: minus 7%. I did not bother with width 16,
doubling had already shown throughput was not width-bound.
Instead of guessing again I instrumented per-phase wall times (the
phase_*_mscommit in this PR). The breakdown: references confirm 6 min,definition pass 12 min, hover/hierarchy sweep 7 min, and the server burning
6.5 cores sustained for the whole pass, on a 32-thread box with plenty of
idle capacity. So the ceiling was not my machine and not gortex's width.
The server itself stops scaling on this solution (Roslyn-side serialization
and GC under FindReferences traffic), far below what the hardware could
feed it.
The bottom of the hole:
textDocument/definitioncosts ~15.5ms per requestwhile references/incomingCalls traffic runs in the same process, and ~1ms
without it. The FindReferences-backed requests were not just their own 6
minutes, they taxed every other request the server answered. Dropping them
did not shave the pass, it collapsed it: 26 minutes down to 5.6, identical
type coverage.
Two lessons worth keeping even if the diff changes shape. The lever was the
request mix, not the concurrency knob. And both of my "obvious" fixes were
built on a wrong guess about where the minutes lived, which one run with
phase timing killed. Numbers are from a strong box (specs at the end), so
scale accordingly. That is exactly why the cheap mode matters.
Why
csharp-ls's
textDocument/referencesandcallHierarchy/incomingCallsbothride Roslyn's FindReferences machinery. Two costs:
request. A full enrichment pass grew the server past 30GB and it never
came back. Every released csharp-ls in the wild still has this.
production C# codebase (~6,000 files / ~117k symbols) the two request
classes cost 6 min of wall directly, and taxed everything else: with
them in-process
textDocument/definitionaveraged 15.5ms/request,without them ~1ms. The pass is server-bound, so that tax is most of the
wall time.
What
ServerSpec.NoHeavyRequests(set on the csharp spec): the referencesconfirm sweep and the incomingCalls leg never run. Sited ambiguous edges
route through the existing definition-rebind pass instead.
textDocument/definitionat the call site reaches the sameconfirm/rebind verdicts at position-request cost.
type target (
findEnclosingTypeNamed).(interface/abstract), the compiler-proven edge to the declared member
is added and the stored impl-edge inference stays exactly as it was.
Unrelated misbinds still rebind. Note this arm (and the ctor-confirm
one) lives in the shared definition-rebind pass, so it applies to every
server's fallback, not just the opt-out ones: a case that previously
REBOUND an impl-targeted edge onto the declared member now adds the
declared edge and keeps the inference. That is deliberate — the impl
edge is a devirtualization guess definition cannot vouch for either
way — but it is a behavior change on the default path too.
GORTEX_LSP_HEAVYenv override, wins both ways:onrestores the heavylegs (for servers carrying the #410 fix),
offforces the opt-out. Nonew config keys.
maxParallel, grouped by call-sitefile (one session acquire and one verdict cache per file). It used to run
serially. Under the opt-out it carries the whole confirm load. Note this
applies to every provider, not just csharp. It mirrors the pattern the
references confirm sweep already uses, and the suite is green, but my
real-world runs are C# only.
(
phase_*_ms: setup / impls / confirm / definitions / refs-add / sweep).The aggregate duration cannot say which pass owns the minutes.
Measured (production C# codebase, csharp-ls with the #410 fix; test box below)
Edge-by-edge store diff between the two modes (same binary, back-to-back
full passes on the same tree):
So the direct LSP delta is ~40k facts, but it amplifies to ~+11% of the
whole edge set through the dispatch fan-out. The fan-out is INFERRED tier
and filterable. It is also exactly the population a background heavy pass
would restore after a fast cold index, which is why the backfill-lane
follow-up matters.
Recall check on the counted fixture repo (cs-probe): definition-based
verdicts match references-based verdicts 108/120 exact plus 12 explained (4
ctor-vs-type, 8 interface-vs-impl where definition is the more honest
answer). Fixture hand-count of the related outgoingCalls payloads: 97/97.
The recall gap on the production codebase is the incoming-driven dispatch-add
population. Heavy mode remains available for it (env override today, a
background backfill lane is a follow-up discussion).
Width note: the pass is server-throughput-bound, not slot-bound. maxParallel
6 to 12 moved wall only 7% with the heavy legs on, while the machine had
idle threads to spare. The win comes from not asking for the expensive
work, not from more concurrency.
Interplay
those servers still leak per references/incoming request.
GORTEX_LSP_HEAVY=on: as of this PR noreleased csharp-ls contains the #410 fix. Every release up to and
including 0.26 leaks unboundedly per references/incomingCalls request
(~10MB/req on a large solution, a full pass exceeds 30GB RSS). Only a
build from csharp-ls main after #410 is safe for the heavy legs. If in
doubt, leave the default alone.
GORTEX_LSP_HEAVY=onrestores the deep pass.Version-gating the default once a release ships with the fix is a
follow-up.
hover-class cost). Every csharp-ls today answers it with an empty stub,
harmless but zero data. csharp-ls #412 (filed) implements it for real,
and with that build the outgoing payloads feed the dispatch add path.
That was worth +6.8 points of coverage (67.7% to 74.5%) on the same
codebase, in both modes. So "use latest csharp-ls" pays twice: #410 makes
the heavy legs safe, #412 makes the cheap outgoing leg productive.
Follow-ups (open questions, not this PR)
Is an LSP sidecar still needed? Before this work the plan was a
separate long-lived server process gortex talks to but does not own, mostly
to escape the cold-pass cost. At 5.6 minutes (on my box, on the repo I work
on — bigger solutions and ordinary hardware will still see multiples of
that) the cold pass stopped being the main argument. What is left of the
sidecar case, honestly:
fan out to ~+11% of the edge set through dispatch synthesis) is real and
nobody wants to wait 25 minutes for it. A background lane that runs ONLY
the heavy legs against the already-confirmed graph would restore it at
leisure. That lane needs a pass mode that skips hovers/definitions and
re-verifies only what is still heuristic — machinery that does not exist
yet.
on my box) before any request works. A sidecar that outlives the daemon
keeps the Roslyn workspace warm. That is the strongest remaining sidecar
argument, and it matters more for the background lane than for cold
indexing.
nobody would spend foreground time on. Cheap to explore once a
background lane exists.
So: probably yes to a background heavy lane, maybe to a sidecar, and
neither blocks this PR.
Server-side headroom. The csharp-ls side is not done either — profiling
shows hot request paths that do not yet use Roslyn's indexed lookups, so
per-request costs on the heavy legs should drop further upstream. Faster
heavy legs shrink the gap this PR routes around; the two-mode design keeps
working either way, it just makes the deep mode cheaper.
Unmeasured territory. All numbers here come from one fairly ordinary
(if large) application codebase. The specialities are untested: heavy
source-generator usage, designer/codegen-dominated projects, Razor — those
tax the server's solution load and per-request costs in ways none of these
runs measured. Expect different ratios there, not just different absolute
times.
Test box
For context on the absolute numbers (the relative deltas are the part that
transfers):
max_parallel12 for these runsDifferent hardware lands on different absolute times. The gap between the
two modes is what carries over, and it is why the cheap mode is the
default.
Last remark
This was a long and painful process, two full days of probes, wrong turns
and reruns across two codebases, so there may well be points I missed or
got only half right. If something here does not add up, ask. I have the
raw numbers and the probe scripts for all of it.