Repository navigation
Compile reducible custom expression node kinds - #579
Conversation
|
The collector and emitter each reduce the same custom node independently. If |
2923d1d to
fcc7022
Compare
|
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. |
|
Follow-up: #584 extends the collected-tree reuse to ordinary The new regressions reproduce strict compilation failures on this parent when 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. |
|
One remaining gap: collection caches reductions for all reducible unknown node kinds, but emission handles |
|
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
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.
08b190e to
183ea00
Compare
|
I still do not know what reduce or extension is but PR look legit, merging |
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_systemfor 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
65ef6641cbe709624d713b1db5248c4b877fbd13on 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
TryReduceNodehelper (renamed fromTryReduceCustomNodebecause 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
08b190e9versus its preceding #579 headfcc70222are 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.