Skip to content

Compile reducible custom expression node kinds - #579

Merged
dadhi merged 3 commits into
dadhi:masterfrom
jogibear9988:fix/custom-node-reduction
Nov 4, 2026
Merged

dadhi merged 3 commits into
dadhi:masterfrom
jogibear9988:fix/custom-node-reduction

Conversation

@jogibear9988

@jogibear9988 jogibear9988 commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

A valid System expression node with a custom NodeType and CanReduce=true currently returns null from strict FEC compilation. Reduce unknown System nodes through ReduceAndCheck, then use the existing collection/emission paths. Ordinary Extension nodes in both expression APIs also reuse their collected reductions. Reduction chains retain captured constants, and wrong-type reductions receive System's ArgumentException.

Both passes reuse the exact same reduced tree, addressing dadhi's review comment. A reducer may create fresh locals, captured objects, or nested lambdas on every call; independently reducing during emission loses the identities recorded during collection. A lazy cache in the lambda's compiler context stores reductions by reference identity. It is discarded after compilation; it is not shared between compilations. No cache object is allocated for ordinary expression trees. The cold helper and FEC's existing SmallMap implementation keep this outside the common visitors. Extension reduction reuse also applies to LightExpression. Its intrinsic collection/emission hooks retain their existing dispatch and do not call Reduce.

This is an independent extraction from #558, based directly on master 65ef6641cbe709624d713b1db5248c4b877fbd13, with separate commits for custom-node reduction reuse and shared Extension reduction reuse (183ea000, brought in from #584). It does not depend on the other standalone fixes. #558 and #559 are unchanged.

Validation

  • Eight custom-node regression methods pass on .NET 8, .NET 10, and .NET Framework 4.7.2. The new fresh-block/local/captured-object regression was run before the correction: strict compilation failed and Reduce was called twice. It now returns the expected result and calls Reduce once.

  • The Extension identity fixture adds seven shared methods for Standard/LightExpression plus one Light intrinsic test. It covers fresh block locals and captured objects, nested lambdas, labels, reference identity, separate compilations, direct emission paths and invalid first reductions. All pass on .NET 8, .NET 10 and .NET Framework 4.7.2.

  • The existing progress regression now asserts reuse of the valid first collected result; a reducer is not called again during emission. Null/self first reductions remain rejected.

  • Additional coverage checks fresh nested lambdas, repeated use of the same node, distinct nodes overriding equality, independent repeated compilations, reduction chains, invalid reduction validation, and emission without collection. The new tree-identity regressions exercise default, no-inlining, and strict/debug compilation flags.

  • Full Release solution build passes with zero warnings/errors.

  • Legacy native suites pass: 1,756 cases on .NET 10, 1,755 on .NET 8, and 1,639 on Framework.

  • Complete Framework runner passes 217 TestX + 1,639 legacy cases. en-US culture is used for existing English exception-message assertions.

  • On .NET 8/10, the full runner still fails ParityTests.Binary_operators_with_methods_and_nullable_operands_are_the_same_as_system for nullable MultiplyChecked in both input variants. These are the previously documented baseline failures; unmodified master was not rerun for this rebase. No other fixture failures were observed; legacy suites were invoked separately because this existing TestX failure gates the normal runner.

Rebase

Rebased onto freshly fetched upstream master 65ef6641cbe709624d713b1db5248c4b877fbd13 on November 4, 2026. Both test runners retain master's RuntimeTypeConstantRegressionTests and this PR's custom-node/Extension regression registrations. All three logical commits are preserved: 472e5be3, 0559f547, 183ea000. Range-diff confirms the only rebase adjustments are test-runner registration context; production changes and regression fixtures are unchanged.

Validation above was rerun after this rebase. Performance was not remeasured, and no new timing claim is made for this head.

Review follow-up: Extension reduction identity

Addresses dadhi's comment. Collection and emission both route ordinary Extension nodes through the shared TryReduceNode helper (renamed from TryReduceCustomNode because it now serves both node categories). The same compilation-local, reference-identity cache serves both expression APIs; System-only custom kinds retain CanReduce/ReduceAndCheck validation and Extensions retain their existing null/self rejection behavior.

This includes the complete follow-up fix originally proposed in #584; that PR is superseded by this one and does not need a separate merge.

The earlier measurements for the pre-rebase Extension fix 08b190e9 versus its preceding #579 head fcc70222 are documented in #584. They recorded an extra 120 B per tested single-Extension compilation, including stable reducers. Ordinary non-reducing controls retained their compilation allocations; tested controls retained identical emitted IL and invocation allocations. Larger reduction sets may require more map storage. These measurements were not rerun for this integration, and are not comparisons against current remote/master. The cache is discarded after compilation.

dadhi commented Nov 3, 2026

Copy link
Copy Markdown
Owner

The collector and emitter each reduce the same custom node independently. If Reduce() constructs fresh expression objects (for example, a block with new ParameterExpression locals each time), collection records locals from one reduced tree while emission sees another; local lookup is identity-based, so strict compilation can fail or emit incorrectly. Please preserve/reuse the reduced tree from collection for emission (or otherwise ensure both passes consume the same reduction result). Add a regression whose reducer creates a fresh block/local on every Reduce() call.

@jogibear9988

Copy link
Copy Markdown
Contributor Author

Fixed in fcc70222. Collection and emission now reuse the exact same reduced tree. A lazy cache in each lambda's CompilerContext stores the result by node reference identity using FEC's existing SmallMap. Emission reads that stored result, so it sees the same locals, captured constants, and nested lambdas recorded during collection. The cache belongs to that compilation and is allocated only when a reducible custom node is encountered.

Added the requested fresh-block regression: every Reduce() call creates a fresh block, local, and captured object. Before the fix, strict compilation failed and Reduce() ran twice. With the fix it returns 42 and asserts exactly one reduction. Additional cases cover fresh nested lambdas, shared nodes, distinct nodes overriding equality, separate compilations, and emission without collection.

Rebased onto current master (750e1e8), preserving the upstream reduction-progress guards and tests. All eight regression methods pass on .NET 8, .NET 10, and .NET Framework 4.7.2. Release build is clean; Framework and legacy suites pass. The existing nullable MultiplyChecked parity failures remain in the modern full runners. The PR description now reflects the rebased validation and removes timings measured against the older base.

@jogibear9988

Copy link
Copy Markdown
Contributor Author

Follow-up: #584 extends the collected-tree reuse to ordinary ExpressionType.Extension nodes in both Standard and LightExpression. It is based on this PR's exact head (fcc70222) and adds one follow-up commit; this PR is unchanged. Please merge #579 first. The follow-up description links the incremental diff because its formal GitHub base is master.

The new regressions reproduce strict compilation failures on this parent when Reduce() creates fresh locals/captured constants. They pass with the follow-up on .NET 8, .NET 10, and .NET Framework 4.7.2, with additional coverage for nested lambdas, shared labels, reference-identity keys, separate compilations, emission APIs, invalid first reductions, and Light intrinsic hooks. Existing nullable MultiplyChecked parity failures remain unchanged.

The measured tradeoff is +120 B per single-extension compilation for the lazy cache. Ordinary controls retain compilation allocations; all measured controls retain identical emitted IL and invocation allocations. Compilation timings, including slower observations, are documented in #584.

dadhi commented Nov 4, 2026

Copy link
Copy Markdown
Owner

One remaining gap: collection caches reductions for all reducible unknown node kinds, but emission handles ExpressionType.Extension earlier (around TryEmit’s extension case) and calls expr.Reduce() directly. An extension node whose Reduce() creates fresh locals or nested lambdas is therefore still reduced separately in the two passes, so the same identity mismatch can occur. Please route the extension case through TryReduceCustomNode(expr, ref context) too, and add a fresh-local regression with NodeType == ExpressionType.Extension.

@jogibear9988

Copy link
Copy Markdown
Contributor Author

You're right: the ordinary Extension cases still bypassed the reduction cache. This is the same identity gap addressed by the follow-up in #584, so I have brought its fix commit 08b190e directly into #579; #584 now contains no additional change that needs a separate merge.

Both collection and emission route Extension nodes through the shared helper, renamed TryReduceNode now that it serves both ordinary Extensions and custom System node kinds. The compilation-local cache keeps the exact reduced tree by reference identity for both Standard and LightExpression. System custom-node validation, Extension null/self rejection, and Light intrinsic hooks are preserved.

ExtensionReductionIdentityRegressionTests.Fresh_blocks_and_captured_constants_share_the_collected_tree explicitly uses NodeType == ExpressionType.Extension, creates a fresh local and captured object on every Reduce call, checks the returned value, and asserts one reduction. Additional tests cover fresh nested lambdas/labels, shared versus distinct nodes, repeated compilations and direct emission APIs. The valid-first-reduction progress test now checks reuse; invalid first reductions remain rejected.

Validation: clean Release solution build; focused custom-node, Extension-identity and reduction-progress tests pass on .NET 8, .NET 10 and .NET Framework 4.7.2. Legacy suites pass (1,756 / 1,755 / 1,639); Framework passes 213 TestX cases. Full modern runners retain only the known nullable MultiplyChecked parity failure in both APIs.

The allocation tradeoff remains documented: the existing measurements of this exact commit against fcc7022 recorded +120 B for each tested single-extension compilation, including stable reducers. Ordinary controls retained compilation allocations; measured emitted IL and invocation allocations were unchanged. These measurements were not rerun during this integration.

Use CanReduce and ReduceAndCheck for unknown System expression node kinds in the existing collector and emitter. Keep validation in a private cold helper; preserve label operand scope. Test reduction chains, captured constants and invalid result types.
Keep reduced trees in a lazy per-compilation reference-identity cache so collection and emission observe the same locals, constants, and nested lambdas. Preserve ReduceAndCheck validation and leave LightExpression unchanged.

Add regressions for fresh reductions, shared and distinct node identities, repeated compilation, and emission without collection. Verified all regression cases on net8.0, net10.0 and net472; full Release build passes. Existing master nullable MultiplyChecked parity failures remain unchanged.
Extend the lazy per-compilation reduction cache from dadhi#579 to ExpressionType.Extension for Standard and LightExpression. Collection and emission use the same reduced locals, constants, labels and nested lambdas. Preserve System validation for custom node kinds and existing null/self reduction rejection; intrinsic callbacks remain unchanged.

Retain the parent per-node reference-identity policy: shared nodes reduce once per lambda compilation, distinct nodes stay separate, and later compilations get fresh reductions. Update the existing progress regression to assert reuse of a valid first reduction instead of requiring a second emission-time reduction.

New regressions reproduce strict compilation failures on the parent and pass on net8.0, net10.0 and net472. Full solution, legacy suites and Framework runner pass; modern full runners retain only the existing nullable MultiplyChecked parity failures. Measured working controls emit identical IL and retain invocation allocations. Single-extension compilation adds a 120-byte lazy cache; ordinary controls retain compilation allocations.
@jogibear9988
jogibear9988 force-pushed the fix/custom-node-reduction branch from 08b190e to 183ea00 Compare November 4, 2026 19:49
@dadhi

dadhi commented Nov 4, 2026

Copy link
Copy Markdown
Owner

I still do not know what reduce or extension is but PR look legit, merging

@dadhi
dadhi merged commit 50ace74 into dadhi:master Nov 4, 2026
4 checks passed
@jogibear9988
jogibear9988 deleted the fix/custom-node-reduction branch November 4, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants