perf(equivalence): avoid LINQ allocation in FindBodyOwner - #97
Conversation
FindBodyOwner used AncestorsAndSelf().FirstOrDefault(predicate) to walk up to the declaration owning a mutated node's body. Because ClassifyUnreachableCode calls HasThrowOnlyBody, which calls FindBodyOwner, ahead of every check except the cheap no-op, constant-folding and regex shorthand checks, this LINQ chain ran for essentially every real mutation candidate. Each call allocated the enumerator state machine produced by AncestorsAndSelf plus a delegate for the predicate, on a hot path that offers nothing back for the allocation. Replace the chain with a manual loop that walks node.Parent and pattern-matches the same set of declaration kinds, returning as soon as one matches. The traversal order, stopping conditions and return value are unchanged for every input; only the allocations are gone. Add tests pinning FindBodyOwner's behavior through HasThrowOnlyBody for the two owner/body-shape combinations that were not yet exercised by the existing throw-only-body tests: a local function reached through its block body instead of an expression body, and an accessor reached through its expression body instead of its block body.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
=======================================
Coverage 95.26% 95.26%
=======================================
Files 88 88
Lines 5850 5852 +2
Branches 1273 1273
=======================================
+ Hits 5573 5575 +2
Misses 122 122
Partials 155 155 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
FindBodyOwnerinEquivalenceClassifierusednode.AncestorsAndSelf().FirstOrDefault(predicate)to find the innermostdeclaration owning a mutated node's body. This method is reached from
HasThrowOnlyBody, whichClassifyUnreachableCodecalls ahead of everycheck except the cheap no-op, constant-folding and regex-shorthand checks
in
Classify's dispatch chain — so it runs for essentially every realmutation candidate processed. Each call allocated Roslyn's
AncestorsAndSelf()iterator state machine plus a delegate for thepredicate, on a hot path where neither allocation buys anything.
This change replaces the LINQ chain with a manual
for (var current = node; current is not null; current = current.Parent)loop using the same pattern-matching checks and an early return. The
traversal order, stopping conditions, and return value are identical for
every input — this is a pure performance fix with no behavior change.
The sibling LINQ chain in
ClassifyConstantOnlyContext(around line 1276)is intentionally left untouched; it is being addressed by a separate,
independent change.
Test coverage added
The existing throw-only-body tests already covered most owner/body-shape
combinations (block method, block accessor, expression-bodied property,
expression-bodied indexer, expression-bodied local function). Two
combinations were still missing and are added now:
expression body)
block body)
Both exercise
FindBodyOwner's traversal with the mutation sittingseveral ancestors below the eventual owner, confirming the manual loop
stops on the exact same node the old LINQ chain did.
Test plan
dotnet test ./Frameshift.slnx— all 29376 tests passdotnet csharpier format .— working tree clean afterwardtraversal or stopping condition were wrong (verified by reasoning
through the owner-kind switch, since the fix is behavior-preserving)