You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix workspace-cache segfault; harden, measure and speed up the language server - #129
Loading a workspace killed the language server with an access violation (0xC0000005) — a forked process, so a segfault isn't catchable by JS or N-API. Discovered on an old script base still using legacy syntax.
The compiler bug itself is not fixed and still exists (native/polserver segfaults on analysis-past-errors of a local declaration inside a case branch — see Known issues). escript.continueAnalysisOnError: false exists precisely to avoid triggering it, and setting it does work. What's fixed here is that the workspace-cache build wasn't honoring it: updateCache() calls buildReferences() with no argument for every script in the workspace, and both native entry points hardcoded their fallback to true regardless of the user's setting — so the one path that compiles every file in the workspace always ran in the unsafe mode, even for someone who had explicitly disabled it. Both now fall back to gExtensionConfiguration.continueAnalysisOnError, so the setting is honored everywhere.
1240-script workspace, continueAnalysisOnError: false: cache build previously died at file 47 (0.6s) despite the setting → now completes in 9.1s. (The default is true; a workspace containing the trigger pattern still crashes on it unless the user opts out.)
2. Crash diagnostics
NODE_ADDON_API_CPP_EXCEPTIONS_ALL was never defined, so only Napi::Error converted; any other std::exception hit std::terminate() with no message. Fixed.
Added uncaughtException/unhandledRejection handlers, Node diagnostic reports, an in-flight-request breadcrumb, and escript.trace.server.
Release now emits a PDB (/Zi, /OPT:REF//OPT:ICF kept so the binary is unchanged); CI archives it, the .vsix still ships only the .node.
Bug fixed:XmlDocParser tested paramValue twice and never paramName, null-deref on a <parameter> with no name — reachable from an ordinary hover.
3. Performance
Round 1 — compile path
Change
Effect
Configure + prune the em/inc parse-tree caches (never called before)
9.2ms → 1.6ms per analyze()
Analyze lazily instead of on every change notification
~2 compiles/keystroke → 1
Cache dependency lists in JS, flat sorted Uint32Array for tokens
SemanticContextBuilder::visitChildren now skips any rule context whose range doesn't contain the caret (guarded on stop token, so half-typed code still completes).
Also: get_contents() reads unopened files in C++ instead of round-tripping to JS; indexed Set() over push() in Diagnostics/Dependents/Completion/References; honors CancellationToken; yields on a time budget in the index build; dropped dead CacheCompiledScripts.
Correctness bugs found while measuring:
BuildReferences() reported into the member reporter instead of its own (uncleared) local one — every workspace-cache build leaked every file's diagnostics into that document's persistent reporter, and diagnostics() answered stale for documents never opened.
BuildReferences() never pruned the parse-tree caches — the one path touching every file grew both to the whole include set.
reopen() left the reference index unbuildable: the JS memo outlived the native cache clear, so after an ecompile.cfg change updateCache() served its already-resolved promise and the index silently never rebuilt.
4. Packaging and startup
Before
After
.vsix entries
817
16
Uncompressed
6.86 MB
3.90 MB
JS payload
~4.3 MB / ~750 files
1.25 MB / 2 files
Bundled with esbuild instead of shipping raw tsc -b output next to three copies of node_modules. .vscodeignore tightened. activationEvents: ["onDebug"] narrowed to onDebugResolve:pol/onDebugDynamicConfigurations:pol — it previously fired for any debug session. Activation no longer forks the server unconditionally: .cfg binds to the language, so opening any unrelated setup.cfg used to fork Node and load the 2.59 MB addon before discovering there was no pol.cfg. LTO enabled for Release (~48 KB smaller; contribution otherwise unmeasured).
Reviewer notes
Behaviour changes worth knowing:
getContents returns undefined for a document that isn't open now; the addon reads the file itself.
Cancelled requests answer RequestCancelled, not an empty result (several drive persistent editor state).
reopen() callers must call invalidateReferenceCache() — couldn't wrap reopen() itself, N-API DefineClass methods land non-writable on the prototype.
main moved to ./dist/extension.js, server to dist/server.js; addon still loads after the crash handlers install.
New escript/logProfile notification dumps compiler counters (no client UI, for scripted measurement); GetProfile now exposes all of Profile.
Known issues / not in this PR
local-in-case segfault is unfixed — compiler bug, still crashes anyone on the default continueAnalysisOnError: true, and is why npm run test-all stays red (testsuite/escript/spread/resource-tracker-functional.src).
Reference index never clears — referenced_by only grows, returns stale ranges after edits, and is why LSPWorkspace::_cache can't evict.
tokenize() is ~25% of analyze() and discarded unless semantic tokens are served; it's private, so skipping it needs a submodule change.
em/inc caches save parsing only — the AST is rebuilt on every hit.
Semantic token deltas (full.delta) would be the largest remaining per-keystroke win, but change the advertised capability.
AsYlum-
changed the title
Fix workspace-cache segfault; add crash diagnostics; speed up analysis
Fix workspace-cache segfault; harden, measure and speed up the language server
Sep 16, 2026
updateCache() calls buildReferences() with no argument for every script in
the workspace, and both native entry points hardcoded their fallback to
`true`, so the setting was ignored on the one path that compiles everything.
That matters because continuing past errors segfaults the compiler on a
`local` declaration inside a `case` branch:
case (x)
1:
local y := 1;
endcase
On a 1240-script workspace the cache build died at file 47 after 0.6s; it
now completes in 9.1s. The compiler bug itself is unfixed and still
reachable on the default setting.
- Define NODE_ADDON_API_CPP_EXCEPTIONS_ALL. Without it only Napi::Error was
converted to a JS exception; any other std::exception escaping an N-API
method reached std::terminate() and aborted with no message.
- Add uncaughtException/unhandledRejection handlers, Node diagnostic
reports, and a breadcrumb naming the in-flight request. server/connection
is required after they are installed, since a static import would be
hoisted above the native addon load it is meant to cover.
- Fix XmlDocParser testing paramValue twice and never paramName, which
constructed a std::string from nullptr on a <parameter> with no name.
- Emit a PDB for Release builds, archive it in CI, ship only the .node.
- Log stacks instead of ${ex} or e.message.
- Add escript.trace.server; correct the output channel name in README.
- Re-push configuration only when an escript setting changed.
- Configure and prune the em/inc parse tree caches. They were constructed
but never configure()d, and load() bypasses the cache while keep == 0, so
every analyze() reparsed the whole include chain. 9.2ms -> 1.6ms per
analyze on a two-include fixture. Cleared when an .em/.inc changes, since
get_contents() serves unsaved editor text.
- Analyze lazily: mark dirty on change and compile once on the next request
that needs the tree. A keystroke previously cost at least two compiles of
the document and of every open file including it.
- Track dependencies in JS instead of calling dependents() across N-API for
every open document on every diagnostics pull.
- Return semantic tokens as a flat Uint32Array sorted in C++, replacing six
push calls per token.
- Release a document's syntax tree on didClose. The workspace cache never
evicted, so every file ever opened stayed resident.
- Use the error_code filesystem overloads in recurse_collect, which walked
the POL tree with throwing calls, and guard the remaining native call
sites so a failure degrades one request.
- Expose the compiler's existing Profile counters.
- get_contents() called into JS for every file the compiler loads, and the
JS side answered with readFileSync for anything not open in an editor.
That is an N-API round trip plus a sync read plus two string copies per
file, on the path walked by every include of every analysis and by every
file of the workspace index build -- almost none of which are open. JS now
answers only for open documents, where the buffer may hold unsaved edits,
and anything else falls through to SourceFileLoader::get_contents, which
is what ecompile already uses. It is also the precondition for ever moving
compilation off the main thread: while this override calls into JS, no
compile can run on a worker.
- Expose all of Profile, not the 10 fields of 23 that were reachable. The
ones that were missing are the ones that became interesting once parsing
was cached: optimize, disambiguate, tokenize, the per-stage ast* timers,
and ambiguities. Measured with them, tokenize() alone is ~3.1ms of a
~12.5ms analyze() on a 1600-line file -- a quarter of every analysis,
discarded unless semantic tokens are being served -- and astEm dominates
the rest, confirming the caches save parsing but not AST construction.
Two read badly without warning: astSrcMicros and astIncMicros are
decremented by nested include time, so they are self-time and can go
negative, and nothing resets any of them, so a caller measuring one
operation has to diff two snapshots.
- Drop CacheCompiledScripts. It re-walked the entire POL tree with
recurse_collect rather than reusing the list AutoCompiledScripts already
memoizes, and nothing in server/src ever called it.
- updateCache() memoizes its task and deliberately leaves the entry in place
once it resolves, which is what makes every later textDocument/references
free rather than an N-file recompile. But reopen() clears the native
document cache when the ecompile.cfg folders change, and the memo outlived
it: the caller asked for updateCache() again, the already-resolved promise
answered immediately, and the index was silently never rebuilt for the new
configuration. Workspace references simply stopped working until restart.
Fixed with an explicit invalidateReferenceCache() rather than by wrapping
reopen(): methods defined through N-API's DefineClass land on the
prototype non-writable, so assigning over one throws. updateCache only
gets away with it because it is a new property.
- Yield on a time budget instead of once per file. A 1240-script
distribution spent 1240 macrotask turns purely to let a pending request
through, far more often than a request actually arrives.
- Guard against a superseded build. Invalidation can replace a task while
the old one is mid-loop; it now stops instead of writing into its
replacement, and settling is scoped to the task that owns the progress
reporters so a late finisher cannot clear its successor's.
- Search more than one root for the compiled addon. __dirname moves when
this file is bundled into the server, so the layouts are listed rather
than assumed, and a change fails loudly in the existing error instead of
silently resolving to nothing.
- Widen getContents to return undefined for a document that is not open,
matching the addon, and type the full set of profile counters.
- Answer cancelled requests instead of serving them. VSCode cancels
superseded requests as the user keeps typing -- the hover from two
keystrokes ago, a completion list the editor has already dismissed -- and
every one of them still cost a full compile, because ensureAnalyzed() sits
behind almost every handler. The reply is the protocol's RequestCancelled
rather than an empty result on purpose: several of these drive persistent
editor state (semantic tokens are the file's highlighting, diagnostics are
the Problems panel) and an empty success would blank it, where an error
leaves the client holding what it already had. No advertised capability
changes; the token was always passed, just ignored.
- Log the compiler's counters, which nothing outside the tests had ever
read. Dumped after the workspace cache build, where the interesting split
is, and on an escript/logProfile notification so a scripted LSP client can
sample them mid-session. Zero-valued fields are dropped and micros are
rendered as milliseconds so the line stays readable.
- Stop answering getContents for files that are not open. The addon reads
those itself now; returning them from here cost a round trip per file.
- Invalidate the memoized reference index when reopen() reports folder
changes, regardless of the disableWorkspaceReferences setting, so that
re-enabling workspace references later cannot resurrect a stale index.
- Replace async's eachLimit with a ten-line limiter. The package is ~225 KB
of JavaScript parsed during server boot for a single call site that only
runs when a POL workspace was found and the docs are not already on disk.
The server/package-lock.json diff also corrects a staleness that predates
this change: it recorded node-addon-api ^8.3.0 while native/package.json has
said ^8.3.1 since before this branch.
- BuildReferences() reported into the member reporter rather than the local
one it had just constructed and then ignored, and unlike Analyze() it
never cleared it first. So the workspace cache build appended every
diagnostic of every file in the distribution into that document's
persistent reporter, which nothing ever clears: memory never reclaimed,
and diagnostics() answering with stale results for a document the user had
never opened.
- BuildReferences() also never pruned the parse-tree caches. load() inserts
on a miss without bound and keep_some() is the only thing that enforces
the size configured from ecompile.cfg, so the one path that touches every
file in the workspace grew both caches to the workspace's entire include
set and never trimmed. This is the hole the previous round closed for
Analyze() and left open here; ecompile calls keep_some() after every file
for the same reason.
- Build the result arrays by index instead of calling push() per element.
Each of these looked up "push" on the array and then made a JS call per
item: Diagnostics for the document and every open dependent on every
diagnostics pull, Dependents after every analyze, Completion per
keystroke, References per request. Same treatment tokens() already got.
SemanticContextBuilder::visitChildren recursed into every child
unconditionally, with a dynamic_cast per node, to collect the contexts
containing the caret. A rule context spans all of its children, so the
contexts that can contain a position are only ever a single root-to-leaf
chain -- the rest of the file was walked for nothing, on every hover,
definition, completion, signature help and references request.
Measured on a 1610-line fixture, probing near the end of the file:
hover 1.544 ms -> 0.099 ms (15.6x)
definition 1.376 ms -> 0.046 ms (30x)
The gain scales with file size; on a 17-line fixture the same change is
worth about 35%.
The guard is on the stop token rather than on contains() alone. An
incomplete rule has no stop token, calculate_end_position() gives it end
{0,0}, and contains() is then false for every position -- routine while the
user is mid-keystroke, so such a subtree still has to be descended into or
completion would break on exactly the half-typed code it serves.
Also:
- Add token_length(), for the position tests that only ever wanted a length
and paid for a std::string to get it. Kept to two virtual calls: an
earlier version also fetched the input stream and its size and benchmarked
no faster than getText(), because a token short enough for the string's
small-buffer optimisation never reaches the heap, so the allocation this
was meant to avoid mostly was not happening while the extra dispatch was.
- Stop the completion scan running to end of file. It cannot become a binary
search -- it accumulates the enclosing class/function/enum state on the way
down -- but it can stop once past the caret's line.
- Delete a full copy of the file's token vector in HoverBuilder that was
assigned and never read. get_all_tokens() returns by value, so this
allocated and filled a vector of every token pointer on every hover.
There was no way to measure any of this: no benchmark, no timing, and the
compiler's counters were read by exactly one test. This drives analyze and
each per-request builder over a small fixture and a 1610-line one, and
prints wall-clock per operation alongside the profile delta.
Gated behind JEST_RUN_LONG_TESTS (npm run test-all) like the other slow
suites, since each case runs hundreds of iterations. describe.skip rather
than a no-op gate, because every case here is gated and a no-op would leave
the file with no tests at all, which Jest fails outright.
Two things it asserts despite being a benchmark: that the probe positions
actually resolve a symbol, and that the per-request cases do not move the
compiler counters. A position that misses makes every builder bail
immediately and turns the whole thing into a measurement of nothing, which
is how the first version of it read -- and counters that move mean something
is re-compiling per request rather than reading the analyzed tree.
The large fixture exists because the small one cannot show the parse-tree
walk: with ~17 lines there is nothing to skip.
bscript, plib, libtinyxml and the addon all link statically into a single
.node, so the whole compiler is visible to the optimizer at once. Release
already builds at /O2 /Ob2 /DNDEBUG, so this is the remaining codegen lever.
The binary comes out ~48 KB smaller.
IPO is set on the polserver targets from here rather than inside the
submodule, which stays untouched. antlr is an ExternalProject with its own
build and is not covered.
/GL coexists with the /Zi + /DEBUG + /OPT:REF + /OPT:ICF the previous round
established: dumpbin still matches the PDB to the binary, so a released
crash dump stays symbolisable. That is worth re-checking after any change
here, since an unsymbolisable Release build is the exact problem that setup
was added to fix.
The contribution to run time is not separately measured -- analyze() is
unchanged within run-to-run variance, and the per-request wins in this
branch come from the builder changes, not from this.
Nothing was bundled: the build was tsc -b only, and ts-loader sat in
devDependencies with no webpack config anywhere in the tree. The packaged
.vsix carried 817 entries and 6.86 MB, of which the project's own code was
10 files and 67 KB. The rest was three near-duplicate copies of the
vscode-jsonrpc / vscode-languageserver trees, and the server resolved and
parsed all of it on every boot.
Bundling with esbuild to dist/ brings that to 16 entries and 3.90 MB, of
which 2.59 MB is the addon; the JS payload goes from ~4.3 MB across ~750
files to two bundles totalling 1.25 MB.
vscode and *.node stay external. The addon is located at runtime through a
computed require() that esbuild leaves alone, and native/src/index.ts
already searches the layouts the bundle can run from.
esbuild preserves the one ordering that matters: server/src/index.ts
requires ./server/connection rather than importing it, so the native addon
loads after installCrashHandlers(). The bundle turns that into a lazily
initialised module and the call still happens first. Verified in the output,
and by loading the bundled server.
Packaging fixes that come with it:
- .vscodeignore had no server/node_modules rule at all, so 264 files
shipped, including async/dist/async.js and async.mjs -- 425 KB for one
function. It also excluded **/*.d.ts and then re-included 113 of them, 647
KB, through the client/node_modules negations. None of that is needed now
that nothing ships out of node_modules, so the negations are gone.
- activationEvents said onDebug, which fires for any debug session of any
type in the window. Narrowed to the two events this extension's debugger
contribution actually needs.
- Activation no longer forks the server unconditionally. contributes.languages
binds escriptcfg to .cfg, so opening a setup.cfg in an unrelated project
activated the extension, forked Node and dlopened the 2.59 MB addon before
the server discovered there was no pol.cfg to open. The client now applies
the same test the server already applied in onInitialize, and re-checks it
if a folder is added later.
- Drop ts-loader, which removes webpack's dependency tree from the lockfile.
- Keep a document dirty until analyze() and the dependency refresh
succeed; a throw used to leave an empty tree treated as clean.
- Mark the parse-tree cache stale when an open .inc/.em changes, and
clear it before whichever analysis runs next, so re-analyzing an
including .src first cannot reuse the pre-edit tree.
- On close, dirty documents including the closed buffer and drop the
cached tree of an unsaved .inc/.em.
- After a config reopen with changes, dirty every retained document.
- Drop the analysis before formatting; the formatter runs its own
compiler.
The extension now loads dist/, but the launch task ran only tsc -b -w. 'watch' now runs esbuild in watch mode; 'watch:tsc' keeps type errors in the Problems panel, and the launch waits on both.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Activation cleanup can throw, reference queries can miss pending edits, and native-crash breadcrumbs are not persisted.
No open issues 🎉
3 previously missed issues
In code that hasn't changed since last review
Prevent activation cleanup race from throwing
client/src/extension.ts:135
If a workspace-folder event finishes its POL probe before the activation-time probe, its callback disposes the watcher and sets folderWatch to undefined. When activation resumes, client is set, so this unconditional call throws a TypeError and rejects extension activation. The second client check prevents duplicate clients but not this cleanup race. Use optional chaining here, as in the folder-event callback.
Persist breadcrumbs before entering native code
server/src/misc/Diagnostics.ts:37
beginOperation() only stores the breadcrumb in JS memory. A native access violation or SIGSEGV does not run the uncaughtException or unhandledRejection handlers, so the operation and document are lost for the crash this instrumentation is meant to diagnose. Write the breadcrumb through a crash-surviving mechanism before entering native code rather than relying on describeBreadcrumbs() running afterward.
Index dirty open sources before reference lookup
server/src/server/connection.ts:692
Lazy analysis leaves new references in other open buffers unindexed: this handler analyzes only the queried document, but ReferencesFinder reads the shared referenced_by index populated by each source's analyze(). For example, add a module-function call in an open .src, then find references from the .em before another request analyzes that source. The new call is missing even if the initial workspace cache has completed. Analyze dirty open sources before reading the reference index; this is separate from the known issue of old reference ranges not being removed.
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
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.
AsYlum-
changed the title
1. The crash
Loading a workspace killed the language server with an access violation (
0xC0000005) — a forked process, so a segfault isn't catchable by JS or N-API. Discovered on an old script base still using legacy syntax.The compiler bug itself is not fixed and still exists (
native/polserversegfaults on analysis-past-errors of alocaldeclaration inside acasebranch — see Known issues).escript.continueAnalysisOnError: falseexists precisely to avoid triggering it, and setting it does work. What's fixed here is that the workspace-cache build wasn't honoring it:updateCache()callsbuildReferences()with no argument for every script in the workspace, and both native entry points hardcoded their fallback totrueregardless of the user's setting — so the one path that compiles every file in the workspace always ran in the unsafe mode, even for someone who had explicitly disabled it. Both now fall back togExtensionConfiguration.continueAnalysisOnError, so the setting is honored everywhere.1240-script workspace,
continueAnalysisOnError: false: cache build previously died at file 47 (0.6s) despite the setting → now completes in 9.1s. (The default istrue; a workspace containing the trigger pattern still crashes on it unless the user opts out.)2. Crash diagnostics
NODE_ADDON_API_CPP_EXCEPTIONS_ALLwas never defined, so onlyNapi::Errorconverted; any otherstd::exceptionhitstd::terminate()with no message. Fixed.uncaughtException/unhandledRejectionhandlers, Node diagnostic reports, an in-flight-request breadcrumb, andescript.trace.server./Zi,/OPT:REF//OPT:ICFkept so the binary is unchanged); CI archives it, the.vsixstill ships only the.node.XmlDocParsertestedparamValuetwice and neverparamName, null-deref on a<parameter>with noname— reachable from an ordinary hover.3. Performance
Round 1 — compile path
em/incparse-tree caches (never called before)analyze()Uint32Arrayfor tokensdidCloseRound 2 — per-request path (
native/test/benchmark.spec.ts, 1610-line/200-fn fixture):hoverdefinitioncompletion/tokens/symbols/analyzeSemanticContextBuilder::visitChildrennow skips any rule context whose range doesn't contain the caret (guarded on stop token, so half-typed code still completes).Also:
get_contents()reads unopened files in C++ instead of round-tripping to JS; indexedSet()overpush()inDiagnostics/Dependents/Completion/References; honorsCancellationToken; yields on a time budget in the index build; dropped deadCacheCompiledScripts.Correctness bugs found while measuring:
BuildReferences()reported into the member reporter instead of its own (uncleared) local one — every workspace-cache build leaked every file's diagnostics into that document's persistent reporter, anddiagnostics()answered stale for documents never opened.BuildReferences()never pruned the parse-tree caches — the one path touching every file grew both to the whole include set.reopen()left the reference index unbuildable: the JS memo outlived the native cache clear, so after anecompile.cfgchangeupdateCache()served its already-resolved promise and the index silently never rebuilt.4. Packaging and startup
.vsixentriesBundled with esbuild instead of shipping raw
tsc -boutput next to three copies ofnode_modules..vscodeignoretightened.activationEvents: ["onDebug"]narrowed toonDebugResolve:pol/onDebugDynamicConfigurations:pol— it previously fired for any debug session. Activation no longer forks the server unconditionally:.cfgbinds to the language, so opening any unrelatedsetup.cfgused to fork Node and load the 2.59 MB addon before discovering there was nopol.cfg. LTO enabled for Release (~48 KB smaller; contribution otherwise unmeasured).Reviewer notes
Behaviour changes worth knowing:
getContentsreturnsundefinedfor a document that isn't open now; the addon reads the file itself.RequestCancelled, not an empty result (several drive persistent editor state).reopen()callers must callinvalidateReferenceCache()— couldn't wrapreopen()itself, N-APIDefineClassmethods land non-writable on the prototype.mainmoved to./dist/extension.js, server todist/server.js; addon still loads after the crash handlers install.escript/logProfilenotification dumps compiler counters (no client UI, for scripted measurement);GetProfilenow exposes all ofProfile.Known issues / not in this PR
local-in-casesegfault is unfixed — compiler bug, still crashes anyone on the defaultcontinueAnalysisOnError: true, and is whynpm run test-allstays red (testsuite/escript/spread/resource-tracker-functional.src).referenced_byonly grows, returns stale ranges after edits, and is whyLSPWorkspace::_cachecan't evict.tokenize()is ~25% ofanalyze()and discarded unless semantic tokens are served; it'sprivate, so skipping it needs a submodule change.em/inccaches save parsing only — the AST is rebuilt on every hit.full.delta) would be the largest remaining per-keystroke win, but change the advertised capability.