[EL] DEBUG-4878 Fix null equality in expression language#7991
Merged
dudikeleti merged 3 commits intomasterfrom Jan 20, 2026
Merged
[EL] DEBUG-4878 Fix null equality in expression language#7991dudikeleti merged 3 commits intomasterfrom
dudikeleti merged 3 commits intomasterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7991) 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 (7991) - mean (73ms) : 70, 76
master - mean (73ms) : 71, 76
section Bailout
This PR (7991) - mean (78ms) : 75, 80
master - mean (77ms) : 74, 80
section CallTarget+Inlining+NGEN
This PR (7991) - mean (1,039ms) : 978, 1099
master - mean (1,046ms) : 987, 1106
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 (7991) - mean (114ms) : 108, 119
master - mean (114ms) : 109, 119
section Bailout
This PR (7991) - mean (115ms) : 111, 119
master - mean (116ms) : 112, 120
section CallTarget+Inlining+NGEN
This PR (7991) - mean (767ms) : 697, 836
master - mean (769ms) : 705, 832
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7991) - mean (101ms) : 97, 105
master - mean (100ms) : 95, 104
section Bailout
This PR (7991) - mean (102ms) : 97, 107
master - mean (102ms) : 97, 106
section CallTarget+Inlining+NGEN
This PR (7991) - mean (739ms) : 658, 820
master - mean (731ms) : 669, 794
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7991) - mean (99ms) : 96, 103
master - mean (99ms) : 95, 103
section Bailout
This PR (7991) - mean (101ms) : 97, 105
master - mean (101ms) : 97, 105
section CallTarget+Inlining+NGEN
This PR (7991) - mean (662ms) : 633, 690
master - mean (661ms) : 631, 690
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 (7991) - mean (194ms) : 190, 197
master - mean (198ms) : 192, 203
section Bailout
This PR (7991) - mean (198ms) : 195, 200
master - mean (200ms) : 197, 203
section CallTarget+Inlining+NGEN
This PR (7991) - mean (1,131ms) : 1071, 1191
master - mean (1,144ms) : 1079, 1209
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 (7991) - mean (277ms) : 273, 282
master - mean (284ms) : 272, 296
section Bailout
This PR (7991) - mean (279ms) : 274, 284
master - mean (285ms) : 277, 294
section CallTarget+Inlining+NGEN
This PR (7991) - mean (934ms) : 889, 978
master - mean (939ms) : 887, 990
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7991) - mean (273ms) : 268, 277
master - mean (274ms) : 268, 279
section Bailout
This PR (7991) - mean (272ms) : 268, 275
master - mean (277ms) : 269, 285
section CallTarget+Inlining+NGEN
This PR (7991) - mean (924ms) : 876, 973
master - mean (939ms) : 887, 991
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7991) - mean (271ms) : 265, 278
master - mean (272ms) : 267, 278
section Bailout
This PR (7991) - mean (270ms) : 266, 274
master - mean (274ms) : 268, 280
section CallTarget+Inlining+NGEN
This PR (7991) - mean (830ms) : 814, 846
master - mean (838ms) : 815, 860
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
57d0c9b to
e0ca6a7
Compare
tylfin
reviewed
Jan 7, 2026
tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionParser.Binary.cs
Show resolved
Hide resolved
BenchmarksBenchmark execution time: 2026-01-07 16:02:30 Comparing candidate commit e0ca6a7 in PR branch Found 7 performance improvements and 7 performance regressions! Performance is the same for 157 metrics, 15 unstable metrics. scenario:Benchmarks.Trace.ActivityBenchmark.StartStopWithChild netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CharSliceBenchmark.OriginalCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch net472
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch netcoreapp3.1
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net472
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark netcoreapp3.1
|
GreenMatan
approved these changes
Jan 11, 2026
tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionParser.Binary.cs
Show resolved
Hide resolved
tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionParser.Binary.cs
Show resolved
Hide resolved
e0ca6a7 to
e33f17c
Compare
dudikeleti
added a commit
that referenced
this pull request
Jan 21, 2026
## Summary of changes - Reworked `ProbeExpressionEvaluator` to cache compiled expressions **per runtime type combination** (invocation target, return value, and scope members), instead of a single `Lazy<>` instance shared across all invocations. - Introduced `ExpressionCacheKey` to uniquely identify a compilation context using **runtime types** (`Value.GetType()` when available) and member runtime types to support polymorphic calls. - Replaced single cached fields with `ConcurrentDictionary<ExpressionCacheKey, …>` caches for: - Templates (`CompiledExpression<string>[]?`) - Condition (`CompiledExpression<bool>?`) - Metric (`CompiledExpression<double>?`) - Span decorations (compiled decorations array) - Preserved evaluation semantics and error aggregation, while ensuring compiled delegates match the actual runtime types. ## Reason for change The previous implementation compiled expressions once (on first invocation) and reused them for all subsequent calls. This fails in polymorphic scenarios where: - The declared type differs from the runtime type (e.g., generics/base class/interface method invoked on a derived instance). - Parameters/locals are declared as a base type (or `object`) but carry different concrete runtime types across invocations. In these cases, delegates compiled against the first observed type can fail later (typically invalid casts / expression binding mismatches), causing evaluation errors and incorrect probe results. ## Implementation details - Compute a **runtime-aware cache key** on each `Evaluate()` call: - `thisType`: `InvocationTarget.Value?.GetType()` (fallback to declared type, then `typeof(object)`) - `returnType`: `Return.Value?.GetType()` (fallback to declared type) - `members`: runtime types captured from `ScopeMember.Value?.GetType()` (fallback to declared `ScopeMember.Type`) - Store compiled artifacts in thread-safe caches: - `ConcurrentDictionary<ExpressionCacheKey, ...>` - On cache miss: compile, then `TryAdd` - Added `ExpressionCacheKey` struct: - Precomputes hash over `thisType`, `returnType`, and member runtime types. - Implements equality by comparing `ThisType`, `ReturnType`, and the captured member type sequence (hash used as a fast-path). - Added debug logging for cache misses (type + hash + cache size) to help diagnose compilation churn and verify caching behavior. ## Test coverage - Existing `DebuggerExpressionLanguageTests` remain supported via `CompiledTemplates` / `CompiledCondition` accessors (now returning the first cached entry). - Snapshot Exploration Test ## Dependencies - Depends on: #7991, #7992, #7993 - Merge order: #7991 → #7992 → #7993 → (this PR) ## Other details - Caches may grow with many unique type combinations; however this is required for correctness under polymorphic workloads. If needed later, we can add eviction/size caps. - This PR is part of an effort to make the Snapshot Exploration Test run successfully end-to-end.
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
3 participants
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
Harden binary equality handling (
==/!=) in the probe expression parser to avoid runtime exceptions and produce predictable results when comparing:nullnullwhen the runtime type differs from the compile-time typeImplementation details
==and!=through a dedicatedEqualExpression()helper.non-nullable value typevsnull:falsefor==,truefor!=) since the comparison outcome is deterministic.reference typevsnull:Expression.ReferenceEqualonobject-converted operands to ensure object-reference semantics and avoid type-cast issues.Test coverage
Snapshot Exploration Test
ProbeExpressionParser_NonNullableValueTypeComparedToNull_DoesNotThrow
Other details
This PR is part of an effort to make the Snapshot Exploration Test run successfully end-to-end.