[Dynamic Instrumentation] Fix Equals/GetHashCode contract on Dynamic Instrumentation configuration models#8615
Merged
Conversation
github-actions
Bot
added
area:tracer
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efdd50da52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8615) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (75ms) : 71, 78
master - mean (73ms) : 70, 76
section Bailout
This PR (8615) - mean (77ms) : 74, 81
master - mean (77ms) : 74, 79
section CallTarget+Inlining+NGEN
This PR (8615) - mean (1,108ms) : 1030, 1187
master - mean (1,107ms) : 1048, 1166
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (116ms) : 111, 122
master - mean (113ms) : 109, 117
section Bailout
This PR (8615) - mean (114ms) : 111, 117
master - mean (115ms) : 112, 118
section CallTarget+Inlining+NGEN
This PR (8615) - mean (788ms) : 759, 817
master - mean (794ms) : 766, 822
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (103ms) : 96, 111
master - mean (101ms) : 97, 105
section Bailout
This PR (8615) - mean (103ms) : 100, 105
master - mean (101ms) : 99, 103
section CallTarget+Inlining+NGEN
This PR (8615) - mean (953ms) : 911, 994
master - mean (942ms) : 904, 980
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (101ms) : 95, 107
master - mean (102ms) : 98, 107
section Bailout
This PR (8615) - mean (103ms) : 97, 109
master - mean (101ms) : 98, 105
section CallTarget+Inlining+NGEN
This PR (8615) - mean (825ms) : 782, 868
master - mean (819ms) : 779, 859
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (200ms) : 193, 207
master - mean (198ms) : 192, 205
section Bailout
This PR (8615) - mean (202ms) : 199, 206
master - mean (201ms) : 197, 205
section CallTarget+Inlining+NGEN
This PR (8615) - mean (1,198ms) : 1150, 1246
master - mean (1,195ms) : 1154, 1236
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (288ms) : 282, 294
master - mean (288ms) : 282, 294
section Bailout
This PR (8615) - mean (288ms) : 281, 296
master - mean (287ms) : 281, 294
section CallTarget+Inlining+NGEN
This PR (8615) - mean (964ms) : 946, 981
master - mean (963ms) : 945, 981
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (279ms) : 273, 285
master - mean (279ms) : 271, 287
section Bailout
This PR (8615) - mean (279ms) : 272, 286
master - mean (279ms) : 271, 288
section CallTarget+Inlining+NGEN
This PR (8615) - mean (1,155ms) : 1115, 1195
master - mean (1,157ms) : 1123, 1192
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8615) - mean (278ms) : 271, 284
master - mean (278ms) : 271, 285
section Bailout
This PR (8615) - mean (277ms) : 272, 282
master - mean (278ms) : 270, 285
section CallTarget+Inlining+NGEN
This PR (8615) - mean (1,037ms) : 1000, 1074
master - mean (1,037ms) : 991, 1084
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-05-13 11:59:32 Comparing candidate commit 10f53b7 in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 2 performance improvements and 2 performance regressions! Performance is the same for 47 metrics, 21 unstable metrics, 86 known flaky benchmarks, 40 flaky benchmarks without significant changes.
|
dudikeleti
added a commit
that referenced
this pull request
May 12, 2026
…y serialized Json JToken.DeepEquals on JObject ignores property order (it routes through JPropertyKeyedCollection lookup), but JObject.ToString preserves the original property order. Hashing on Json.ToString() therefore violated the Equals/GetHashCode contract whenever two payloads encoded the same expression with re-ordered keys - the same class of contract bug the previous commit fixed for reference identity. Drop Json from GetHashCode and hash only Str and Dsl. The contract still holds because Equals already requires Str and Dsl to be ordinally equal before it ever looks at Json, so any two Equals-equal segments now also produce equal hash codes. Dsl encodes the same expression as Json in practice, so hash distribution is unaffected for real probe configs. A bonus is that we no longer allocate Json.ToString() on every hash. Adds SnapshotSegmentTests.EquivalentJsonWithDifferentPropertyOrder_ EqualsAndGetHashCodeAreConsistent which fails under the previous hash (Json?.ToString() captures property-insertion order) and passes with the new one, locking in the property-order contract. Addresses chatgpt-codex-connector[bot] review feedback on PR #8615 (discussion_r3227806286). Co-authored-by: Cursor <[email protected]>
jpbempel
reviewed
May 12, 2026
…ation models The auto-generated/manually-written GetHashCode on several Dynamic Instrumentation configuration models compared arrays and a JObject by reference, while the matching Equals methods compared them by content. That violates the Equals/GetHashCode contract for any case where Equals deems them equal but the underlying arrays/JObject are different instances - which is exactly what happens on every RCM apply, since configuration is re-parsed into fresh allocations each time. Fixes: - Add NullableSequentialHashCode<T> helper alongside the existing NullableSequentialEquals<T>, with a T[] overload that avoids the IEnumerator<T> heap allocation and interface dispatch on arrays (the only kind of input we actually pass). - FilterList, Where, ProbeDefinition, SpanDecorationProbe, LogProbe: hash arrays via NullableSequentialHashCode to match the existing content-based Equals. - LogProbe: also include 'When' in the hash, which Equals already considered. - SnapshotSegment: override the record-synthesized Equals/GetHashCode to compare the JObject Json field by content (JToken.DeepEquals / Json?.ToString) instead of by reference. No behavior changes for capture-expression work or any other feature - these are pure correctness fixes split out from a larger branch. Co-authored-by: Cursor <[email protected]>
…merable<T>> and add regression test - Remove the IEnumerable<T> overload of NullableSequentialHashCode. Every caller passes a T[] (Tags, AdditionalIds, Lines, Decorations, Segments, PackagePrefixes, Classes), so overload resolution always binds to the T[] overload anyway. Keeping the IEnumerable<T> version was dead code and a footgun if a future caller landed on a List<T>. - Add ProbeConfigurationComparerTests.CurrentSnapshotsReparsedJsonContent_ ProbeRelatedNotChanged. The test reproduces the RCM re-apply scenario: two LogProbe instances that are content-identical but whose SnapshotSegment.Json fields are independently parsed JObject instances (different references, same content) - exactly what DynamicInstrumentation.Deserialize<LogProbe> produces on every poll. Verified the test fails on master (the previous commit's SnapshotSegment override is the fix that makes it pass), preventing any regression that would re-introduce unnecessary IL re-instrumentation on every RCM cycle. Co-authored-by: Cursor <[email protected]>
…y serialized Json JToken.DeepEquals on JObject ignores property order (it routes through JPropertyKeyedCollection lookup), but JObject.ToString preserves the original property order. Hashing on Json.ToString() therefore violated the Equals/GetHashCode contract whenever two payloads encoded the same expression with re-ordered keys - the same class of contract bug the previous commit fixed for reference identity. Drop Json from GetHashCode and hash only Str and Dsl. The contract still holds because Equals already requires Str and Dsl to be ordinally equal before it ever looks at Json, so any two Equals-equal segments now also produce equal hash codes. Dsl encodes the same expression as Json in practice, so hash distribution is unaffected for real probe configs. A bonus is that we no longer allocate Json.ToString() on every hash. Adds SnapshotSegmentTests.EquivalentJsonWithDifferentPropertyOrder_ EqualsAndGetHashCodeAreConsistent which fails under the previous hash (Json?.ToString() captures property-insertion order) and passes with the new one, locking in the property-order contract. Addresses chatgpt-codex-connector[bot] review feedback on PR #8615 (discussion_r3227806286). Co-authored-by: Cursor <[email protected]>
dudikeleti
force-pushed
the
dudik/di/fix-config-models-equality
branch
from
May 13, 2026 10:54
42eaaf8 to
10f53b7
Compare
jpbempel
approved these changes
May 13, 2026
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 of changes
Equals/GetHashCodecontract on the Dynamic Instrumentation configuration models (FilterList,Where,ProbeDefinition,SpanDecorationProbe,LogProbe,SnapshotSegment).NullableSequentialHashCode<T>helper in.Reason for change
HashCode.Combinewas being called on array properties which inheritobject.GetHashCode/Equals(reference identity).LogProbeadditionally omittedWhenfromGetHashCodeeven thoughEqualsalready used it.Implementation details
T[]overload ofNullableSequentialHashCodeis selected by overload resolution at every existing call site (Tags,AdditionalIds,Lines,Decorations,Segments,PackagePrefixes,Classes). It uses an indexedforloop, avoiding theIEnumerator<T>heap allocation and the interface dispatch (MoveNext/Current) theIEnumerable<T>overload would incur on arrays.Test coverage
ProbeConfigurationComparerTests.ProbeConfigurationComparerTests.CurrentSnapshotsReparsedJsonContent_ProbeRelatedNotChanged