Fix workspace-cache segfault; harden, measure and speed up the language server by AsYlum- · Pull Request #129 · polserver/vscode-escript

AsYlum- · 2026-09-15T22:21:17Z

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/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	fewer N-API round trips
Release syntax tree on `didClose`	was retained for server lifetime

Round 2 — per-request path (native/test/benchmark.spec.ts, 1610-line/200-fn fixture):

Request	Before	After
`hover`	1.544 ms	0.099 ms	15.6×
`definition`	1.376 ms	0.046 ms	30×
`completion` / `tokens` / `symbols` / `analyze`	—	—	unchanged

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.

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.

Copilot

Copilot review overview

🟡 Changes recommended

Cache invalidation, activation races, stale analysis state, and the broken development watch flow can cause incorrect behavior.

Review effort: Balanced
Findings: 8

Open (8)

Prevent duplicate clients during concurrent workspace probes · New
Make debug watch rebuild the bundled extension entries · New
Create diagnostic report directory before configuring reports · New
Preserve dirty state until analysis and dependency refresh succeed · New
Invalidate discarded edits and dependent analyses on close · New
Invalidate parse cache when included buffers change · New
Dirty retained documents after compiler configuration changes · New
Remove redundant compiler pass before formatting · New

What changed in this PR

Hardens the language server against crashes while improving analysis performance, diagnostics, startup, and packaging.

Changes:

Adds lazy analysis, parse-tree caching, cancellation, profiling, and native crash safeguards.
Bundles client/server output with esbuild and reduces packaged extension size.
Improves native diagnostics, reference indexing, semantic operations, and tests.

File	Description
`server/src/workspace/DocsDownloader.ts`	Replaces `async.eachLimit`.
`server/src/server/connection.ts`	Adds lazy analysis, caching, cancellation, and profiling.
`server/src/misc/Diagnostics.ts`	Adds crash breadcrumbs and handlers.
`server/src/index.ts`	Installs handlers before loading native code.
`server/package.json`	Removes `async` dependencies.
`server/package-lock.json`	Updates server dependency lock.
`README.md`	Corrects output-channel name.
`package.json`	Adds bundling and narrows activation.
`package-lock.json`	Replaces webpack tooling with esbuild.
`native/test/native.spec.ts`	Adds cache, release, and XML regressions.
`native/test/benchmark.spec.ts`	Adds native performance benchmarks.
`native/src/index.ts`	Extends native APIs and cache orchestration.
`native/cpp/napi/LSPWorkspace.h`	Declares cache and profiling APIs.
`native/cpp/napi/LSPWorkspace.cc`	Implements safer indexing and caching.
`native/cpp/napi/LSPDocument.h`	Declares document release support.
`native/cpp/napi/LSPDocument.cc`	Optimizes analysis, tokens, and references.
`native/cpp/misc/XmlDocParser.cc`	Fixes missing parameter-name validation.
`native/cpp/compiler/TokenLength.h`	Adds allocation-free token sizing.
`native/cpp/compiler/SemanticContextBuilder.h`	Prunes semantic tree traversal.
`native/cpp/compiler/HoverBuilder.cc`	Removes unused token collection.
`native/cpp/compiler/CompletionBuilder.cc`	Short-circuits token scanning.
`native/CMakeLists.txt`	Adds exception handling, symbols, and LTO.
`eslint.config.js`	Excludes generated bundles.
`esbuild.mjs`	Bundles extension and server entry points.
`client/src/extension.ts`	Starts the server only for POL workspaces.
`client/src/activatePolDebug.ts`	Improves debug-server error reporting.
`.vscodeignore`	Minimizes packaged extension contents.
`.github/workflows/build.yml`	Archives Windows debug symbols.

Files not reviewed (1)

server/package-lock.json: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    const startIfPol = async () => {
+        if (client || !await isPolWorkspace()) {
+            return;
+        }
+        client = startClient(context);
+    };


+		"bundle": "node esbuild.mjs",
+		"build": "npm run grammar && tsc -b && node esbuild.mjs && cd native && npm run build",


    storageFsPath: URI.parse(String(storageUri)).fsPath
 };

+installCrashHandlers(options.storageFsPath);


+        this.dirty.delete(fsPath);
+
+        if (isCachedParseTreeFile(fsPath)) {
+            this.workspace.clearParseTreeCache();
+        }
+
+        document.analyze(this.configuration?.continueAnalysisOnError);
+        this.dependsOn.set(fsPath, document.dependents());


+        const document = this.sources.get(fsPath);
        this.sources.delete(fsPath);
+        this.dirty.delete(fsPath);
+        this.dependsOn.delete(fsPath);


+        // Deliberately does no work here: the next request that needs the tree
+        // triggers exactly one analysis.
+        this.markDirty(fsPath);


+            const hasChanges = this.guard('workspace/didChangeWatchedFiles', () => this.workspace.reopen(), false, ecompileCfg);
+            if (hasChanges) {


        }

-        const formatted = range ?
+        this.guard('textDocument/formatting/analyze', () => this.ensureAnalyzed(fsPath, document), undefined, fsPath);


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

AsYlum- added 11 commits September 22, 2026 19:07

AsYlum- force-pushed the fix/workspace-cache-segfault-and-perf branch from e270ede to 6ab7de3 Compare September 22, 2026 17:21

KevinEady requested a balanced review from Copilot October 1, 2026 08:52

Copilot started reviewing on behalf of KevinEady October 1, 2026 08:53

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Navigation Menu

Repository navigation

Fix workspace-cache segfault; harden, measure and speed up the language server - #129

Fix workspace-cache segfault; harden, measure and speed up the language server#129
AsYlum- wants to merge 11 commits into
polserver:masterfrom
AsYlum-:fix/workspace-cache-segfault-and-perf

AsYlum- commented Sep 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		"bundle": "node esbuild.mjs",
		"build": "npm run grammar && tsc -b && node esbuild.mjs && cd native && npm run build",

		const hasChanges = this.guard('workspace/didChangeWatchedFiles', () => this.workspace.reopen(), false, ecompileCfg);
		if (hasChanges) {

Conversation

AsYlum- commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. The crash

2. Crash diagnostics

3. Performance

4. Packaging and startup

Reviewer notes

Known issues / not in this PR

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AsYlum- commented Sep 15, 2026 •

edited

Loading