[EL] DEBUG-4884 Fix null expression when type is struct#7993
Merged
dudikeleti merged 4 commits intomasterfrom Jan 21, 2026
Merged
[EL] DEBUG-4884 Fix null expression when type is struct#7993dudikeleti merged 4 commits intomasterfrom
dudikeleti merged 4 commits intomasterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
a70f4bc to
0ec0aa0
Compare
BenchmarksBenchmark execution time: 2026-01-07 15:56:11 Comparing candidate commit 0ec0aa0 in PR branch Found 7 performance improvements and 10 performance regressions! Performance is the same for 162 metrics, 7 unstable metrics. scenario:Benchmarks.Trace.ActivityBenchmark.StartStopWithChild net6.0
scenario:Benchmarks.Trace.ActivityBenchmark.StartStopWithChild netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs net6.0
scenario:Benchmarks.Trace.Asm.AppSecEncoderBenchmark.EncodeLegacyArgs netcoreapp3.1
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net472
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net472
scenario:Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery net472
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net6.0
scenario:Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin net6.0
|
0ec0aa0 to
5d12f5e
Compare
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 10 occurrences of : - "evaluationErrors": [
- {
- "expr": "\"1\")",
- "message": "ScrubbedValue"
- }
- ],
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7993) 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 (7993) - mean (73ms) : 70, 77
master - mean (73ms) : 71, 76
section Bailout
This PR (7993) - mean (77ms) : 73, 81
master - mean (77ms) : 74, 80
section CallTarget+Inlining+NGEN
This PR (7993) - mean (1,042ms) : 976, 1109
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 (7993) - mean (112ms) : 107, 117
master - mean (114ms) : 109, 119
section Bailout
This PR (7993) - mean (115ms) : 110, 119
master - mean (116ms) : 112, 120
section CallTarget+Inlining+NGEN
This PR (7993) - mean (773ms) : 696, 849
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 (7993) - mean (102ms) : 98, 105
master - mean (100ms) : 95, 104
section Bailout
This PR (7993) - mean (101ms) : 97, 105
master - mean (102ms) : 97, 106
section CallTarget+Inlining+NGEN
This PR (7993) - mean (733ms) : 658, 808
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 (7993) - mean (100ms) : 95, 104
master - mean (99ms) : 95, 103
section Bailout
This PR (7993) - mean (101ms) : 97, 105
master - mean (101ms) : 97, 105
section CallTarget+Inlining+NGEN
This PR (7993) - mean (665ms) : 634, 695
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 (7993) - mean (194ms) : 190, 198
master - mean (198ms) : 192, 203
section Bailout
This PR (7993) - mean (197ms) : 194, 200
master - mean (200ms) : 197, 203
section CallTarget+Inlining+NGEN
This PR (7993) - mean (1,134ms) : 1067, 1202
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 (7993) - mean (277ms) : 271, 283
master - mean (284ms) : 272, 296
section Bailout
This PR (7993) - mean (278ms) : 274, 282
master - mean (285ms) : 277, 294
section CallTarget+Inlining+NGEN
This PR (7993) - mean (929ms) : 885, 972
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 (7993) - mean (269ms) : 265, 274
master - mean (274ms) : 268, 279
section Bailout
This PR (7993) - mean (270ms) : 265, 275
master - mean (277ms) : 269, 285
section CallTarget+Inlining+NGEN
This PR (7993) - mean (920ms) : 865, 974
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 (7993) - mean (271ms) : 265, 278
master - mean (272ms) : 267, 278
section Bailout
This PR (7993) - mean (270ms) : 266, 275
master - mean (274ms) : 268, 280
section CallTarget+Inlining+NGEN
This PR (7993) - mean (836ms) : 817, 855
master - mean (838ms) : 815, 860
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
5d12f5e to
854b5c3
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
Prevent null-unboxing failures when binding
ScopeMember.Valueinto value-type locals during probe expression compilation.Reason for change
When the target variable type is a non-nullable value type (struct),
Expression.Convert(valueField, type)attempts to unboxnull, which throws at execution time. This caused probe evaluation to fail even when a safe default value would be acceptable.Implementation details
ScopeMember.Value == null→ assigndefault(TValue)Convert(object, TValue)Convert, which already handles null correctly.NullReferenceExceptionfor null value-type bindings.Test coverage
Snapshot Exploration Test
ProbeTests.RecursionWithInnerRefStructTest
ProbeExpressionParser_ValueTypeNull_UsesDefaultValue
Other details
This PR is part of an effort to make the Snapshot Exploration Test run successfully end-to-end.