perf: remove per-node allocations on the parse and validation hot paths

[cdietrich]

Draft, for the discussion in #2233.

Three changes across two independent findings. Both remove a per-AST-node allocation from a hot path; neither changes observable behaviour or a public signature. They are in separate files and can be split into two PRs if you would prefer to take them separately — say the word.

1. Parser: linkContentToContainer and assignMandatoryProperties

packages/langium/src/utils/ast-utils.ts

• linkContentToContainer is called from LangiumParser.parse() with { deep: true }, so it recurses over every node of every document. Per node it allocated an array from Object.entries(node) plus a two-element array per own property, and a closure per array-valued property for forEach. Now a for...in with an indexed inner loop.
• assignMandatoryProperties is called from LangiumParser.construct() for every non-datatype node, and rebuilt an array with Object.values(typeMetaData.properties) each time, although that object is stable and owned by the generated reflection. Now iterates the keys, with the own-property guard your guard-for-in rule asks for.

Measured on a 6516-document / 7 MB workspace. The parse phase has a ±3% run-to-run spread on this machine, so this is the median of six runs with the change alternated on and off back to back, not a single A/B:

phase	before	after	delta
parse all 6516 documents	1163 ms	1101 ms	−5.3% (min-to-min −7.7%)

Standalone per-document parsing, indicative (3 repetitions, not alternated): 619 → 598 ms on 2095 documents of one grammar, 653 → 612 ms on 920 documents of another.

2. Validation: validateAstNodes

packages/langium/src/validation/validation-registry.ts and document-validator.ts

This is the larger of the two. validateAstNodes walks the AST with a generic TreeStream and, for each node, calls getChecks, which builds a fresh four-stage lazy pipeline — stream().concat().filter().map() — overwhelmingly to yield nothing, since most node types have no checks registered.

In our profiles langium/utils/stream.js was the single largest self-time frame on every workload measured, 12.9–16.2% of CPU, and caller attribution charged essentially all of it to validateAstNodes.

• ValidationRegistry gains getCheckArray(type, categories), returning a cached readonly ValidationCheck[]. The registry is static once registration is done, so the list is computed once and invalidated in addEntry. getChecks keeps its exact signature and now returns stream(this.getCheckArray(...)), so nothing public changes. This alone took the stream share from 16.2% to 8.0%.
• validateAstNodes walks with an explicit stack instead of streamAst(...).iterator(), enumerating children exactly as streamContents does and pushing them in reverse so document order is preserved. prune becomes "do not push children when validateChildren is false". It calls getCheckArray, so no stream is constructed per node.

Standalone per-document compiles (parse, then validateDocument), warm, median of 3 after a discarded run:

workload	documents	before	after	delta
metadata documents (enum/primitive/composite/node)	4191	804 ms	691 ms	−14.1%
API definitions	920	507 ms	440 ms	−13.2%
API definitions + one optional category	920	618 ms	541 ms	−12.5%
presentations	1062	675 ms	583 ms	−13.6%
presentations + one optional category	1062	763 ms	673 ms	−11.8%

The gain is largely independent of how many checks are registered — what is removed is the per-node machinery, not the checks.

On a workspace build that parses 6516 documents but validates only 82, this is worth ~2.5%, which is what you would expect.

Verification

• tsc and eslint clean.
• Full Langium suite: 1217 passed, 1 expected fail, 10 skipped — unchanged from before.
• On our side, output verified byte-identical across 8155 compiled artifacts (parse results, diagnostics and serialised output, with and without an optional validation category), by diffing a full corpus run before and after — once for change 1 and again independently for change 2. This mattered most for validateAstNodes, since that change alters how the tree is traversed, so visit order and diagnostic order were exactly what needed confirming.

Open questions

• Is for...in acceptable for AST nodes given they are plain objects built by the parser, or would you prefer a cached key array per $type for assignMandatoryProperties?
• getCheckArray is additive. If you would rather not widen the ValidationRegistry surface, the walk could call getChecks and pay for one StreamImpl per node instead of four — most of the win survives, but not all of it.

[msujew]

I like those changes from a first glance. No API changes, so there'll be little chance of breakage. Do you mind converting this to a non-draft PR? Then I'll have a closer look.

[cdietrich]

Self-review notes before this leaves draft. Verified locally: tsc and eslint clean, full suite 1217 passed / 1 expected fail / 10 skipped.

I also wrote a throwaway differential test that walks every .langium file in the repo and compares streamAst against the new stack walk node-for-node: identical order and identical node identity across all 10 grammars. So the traversal rewrite itself is sound — prune() → "don't push children" is an exact translation, since TreeStreamImpl.prune just pops the child iterator it had already pushed.

Three things I'd like to settle before this is ready for review.

1. for...in diverges from Object.keys, and the own-property guard is on the wrong loop

streamContents enumerates with Object.keys (own properties only). Both of my new AST loops use for...in, which also picks up inherited enumerable properties. That is observable:

const ghost = { $type: 'Ghost' };
const root = Object.create({ ghostChild: ghost });
root.$type = 'Root';
root.realChild = { $type: 'Real' };

[...streamAst(root)].map(n => n.$type)  // ['Root', 'Real']
newWalk(root).map(n => n.$type)         // ['Root', 'Real', 'Ghost']

and linkContentToContainer now writes $container onto that inherited child where it previously did not.

Unreachable for parser-built nodes, which are plain object literals. But linkContentToContainer is exported public API, and the result is that the codebase would hold two disagreeing definitions of "children" — streamContents and these loops.

The asymmetry is the sharper point: I added the hasOwnProperty guard to assignMandatoryProperties, where typeMetaData.properties is a generated plain object literal that cannot carry inherited properties, and omitted it from the two loops over caller-supplied AstNodes, which are the only places it could matter. The guard ended up on exactly the loop that does not need it.

Two honest options:

• Object.keys(node) in an indexed loop. Still removes the per-property two-element arrays and the forEach closures — the bulk of the win — and costs one array per node, with exact parity preserved.
• Keep for...in for the extra speed and change streamContents to match, so there is one definition.

What I would not do is leave the two disagreeing silently. This supersedes my "is for...in acceptable" question below — the answer is that it is fine for the nodes the parser builds, and the real issue is the inconsistency, not the construct.

2. Cache key collides when a custom category contains a comma

validation-registry.ts, categories.join(','). Categories are user-definable ('fast' | 'slow' | 'built-in' | (string & {})). With a category literally named 'a,b':

reg.getCheckArray(t, ['a,b'])      // key: t\0a,b
reg.getCheckArray(t, ['a', 'b'])   // key: t\0a,b  -- same cached array object

I confirmed the second query gets the first one's answer. One-character fix, reusing the separator already there:

const key = categories ? `${type}\u0000${categories.join('\u0000')}` : type;

(Empty-array handling is already correct: [] keys to "Type\0", distinct from "Type".)

3. getCheckArray hands out the live cache entry

readonly is erased at runtime — a caller can push() into the returned array and permanently poison the registry for every subsequent call. Since the method is new, this is the cheap moment to Object.freeze before caching.

Minor

• No packages/langium/CHANGELOG.md entry yet; I will add one.
• My claim that typeMetaData.properties is "stable and owned by the generated reflection" holds for known types only — AbstractAstReflection.getTypeMetaData allocates a fresh { name, properties: {}, superTypes: [] } for unknown ones. Does not affect this change, but the premise is not universal.

On the second open question

I would keep getCheckArray public. entries and addEntry are already protected, so one more getter next to getChecks is not a meaningful widening of the surface, and dropping it would give up the larger half of the win — the stream share went 16.2% → 8.0% from the caching alone.

One further note on the caching: clearing checkCache in addEntry means correctness does not actually depend on the "registry is static once registration is done" assumption in my comment there. addEntry is the only write path to entries, so late registration stays correct. I will reword that comment, since as written it claims a precondition the code does not need.

[cdietrich]

concerns addressed

[msujew]

Thanks, looks good to me 👍