Avoid array covariance checks in ArrayList<T> hot paths#42
Open
lahma wants to merge 1 commit into
Open
Conversation
Profiling the FileParsingBenchmark workload with ETW sampling showed CastHelpers.LdelemaRef at ~1.2% of parse time, driven by interior refs (ldelema) into arrays of reference types in ArrayList<T>: - PushRef (every Add/Push during node list building), - Peek/Pop (token context and label stacks). Changes: - PushRef and the indexer setter obtain the element ref via MemoryMarshal.GetArrayDataReference on net8.0+, which needs no covariance check. Both indices are structurally guaranteed valid (capacity was just ensured / the setter checks against _count). - Peek/Pop read elements by value (ldelem) and clear via a null store, neither of which requires a covariance check on any TFM. GetItemRef/PeekRef/PopRef keep the original ldelema form: duplicating the bounds check there inflated the stack frames of the recursive parser methods and cost ~20% of max parse depth, so ref-returning accessors with non-guaranteed indices are left untouched. Max recursion depth is unaffected (588 vs 588 paren levels on a 1 MiB stack probe). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
CPU profiling of the
FileParsingBenchmarkworkload (ETW sampling at 8190 Hz via ultra) showedCastHelpers.LdelemaRefat ~1.2% of parse time — array covariance checks on interior refs (ldelema) into arrays of reference types insideArrayList<T>, driven byPushRef(everyAdd/Pushduring node list building) andPeek/Pop(token context and label stacks).Changes:
PushRefand the indexer setter obtain the element ref viaMemoryMarshal.GetArrayDataReferenceon net8.0+, which needs no covariance check. Both indices are structurally guaranteed valid (capacity was just ensured / the setter checks against_count).Peek/Popread elements by value (ldelem) and clear via anullstore, neither of which requires a covariance check — this part benefits all TFMs and keeps full bounds checking.GetItemRef/PeekRef/PopRefdeliberately keep the originalldelemaform: an experiment that replaced them with an explicit bounds check +GetArrayDataReferenceinflated the stack frames of the recursive parser methods and cost ~20% of max parse depth (588 → 471 paren levels on a 1 MiB-stack probe), so ref-returning accessors with non-guaranteed indices were left untouched. The final change is at exact depth parity with master (588 vs 588).Expectation management
This is a small win by design — the profile caps it at ~1.2%. Two independent baseline-vs-branch A/B measurements both landed on ~0.9% average improvement on .NET 10 (up to ~2% on individual files), neutral on .NET Framework 4.8, with no regressions in any benchmark of the repository suite. Reporting it transparently so you can decide whether it clears the bar (and verify on your hardware, which past experience says behaves differently).
Correctness
Benchmarks
Full repository suite,
--job medium --runtimes net10.0 net48, AMD Ryzen 9 5950X, Windows 11, .NET SDK 10.0.301. Two independent A/B pairs were measured back-to-back;EsprimaParse(fixed NuGet reference) ran in the same sessions as a machine-noise control. The table shows the cleaner pair (adjacent runs, controls ≤ ±1.8%); "adj" = Acornima delta minus control delta..NET 10.0 (pair 1: adjacent runs)
.NET Framework 4.8 (same pair): adjusted deltas between −0.6pp and +0.9pp, i.e. neutral.
The second A/B pair (different sessions) independently shows the same aggregate: −0.9% average adjusted on .NET 10, ~0 on net48.
ObtainNodeFromIntfBenchmarkis neutral (±0.1% on the iter=10000 cases).No
MethodImplhints were added or changed.🤖 Generated with Claude Code
https://claude.ai/code/session_011pMkJKBxYw3x9VSvbvukjv