[Debugger] Resolve instanceof types at runtime#8785
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
BenchmarksBenchmark execution time: 2026-06-17 14:40:09 Comparing candidate commit 373fbb1 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 64 known flaky benchmarks, 62 flaky benchmarks without significant changes.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes the debugger expression language instanceof operator to resolve target types at runtime (during evaluation) rather than at parse/compile time, enabling expressions to work even when customer types are loaded later in the process lifetime.
Changes:
- Compiles
instanceofinto a runtime helper call (InstanceOfHelper.IsInstanceOf<TValue>(value, typeName)) and adds a resolver that inspects already-loaded assemblies without triggering assembly loads. - Adds a fast-path for common BCL/core runtime types (aliases +
System.*names) to avoid any assembly scanning. - Expands test coverage and updates approval snapshots to reflect the new runtime-resolution behavior (including caching, ambiguity, lazy load, and bounded retry cases).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionParser.General.cs | Switches instanceof compilation from parse-time Type.GetType to a runtime helper call. |
| tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionParserHelper.cs | Fixes reflection-based generic method lookup so internal generic methods (like the new helper) can be found correctly. |
| tracer/src/Datadog.Trace/Debugger/Expressions/InstanceOfHelper.cs | Adds runtime type resolution + caching logic for instanceof without loading assemblies, including BCL fast-path. |
| tracer/test/Datadog.Trace.Tests/Debugger/DebuggerExpressionLanguageTests.cs | Adds regression tests for runtime resolution, caching, ambiguity, and lazy assembly load behavior. |
| tracer/test/Datadog.Trace.Tests/Debugger/ProbeExpressionsResources/Approvals/DebuggerExpressionLanguageTests.IsInstanceOfTrue.verified.txt | Updates snapshot to reflect helper-based instanceof evaluation. |
| tracer/test/Datadog.Trace.Tests/Debugger/ProbeExpressionsResources/Approvals/DebuggerExpressionLanguageTests.IsInstanceOfFalse.verified.txt | Updates snapshot to reflect helper-based instanceof evaluation. |
| tracer/test/Datadog.Trace.Tests/Debugger/ProbeExpressionsResources/Approvals/DebuggerExpressionLanguageTests.IsInstanceOfUnknownType.verified.txt | Updates snapshot to reflect that string is now treated as a known alias at runtime (no longer an “unknown type” parse-time failure). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
@codex review |
![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: b6dfbabf43
ℹ️ 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 (8785) 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 (8785) - mean (74ms) : 71, 78
master - mean (73ms) : 71, 75
section Bailout
This PR (8785) - mean (77ms) : 76, 79
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8785) - mean (1,100ms) : 1055, 1145
master - mean (1,094ms) : 1050, 1139
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 (8785) - mean (115ms) : 109, 122
master - mean (113ms) : 109, 116
section Bailout
This PR (8785) - mean (115ms) : 112, 118
master - mean (116ms) : 110, 122
section CallTarget+Inlining+NGEN
This PR (8785) - mean (797ms) : 775, 820
master - mean (793ms) : 769, 817
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8785) - mean (102ms) : 99, 104
master - mean (104ms) : 98, 111
section Bailout
This PR (8785) - mean (102ms) : 99, 104
master - mean (101ms) : 98, 104
section CallTarget+Inlining+NGEN
This PR (8785) - mean (948ms) : 915, 981
master - mean (949ms) : 908, 991
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8785) - mean (101ms) : 97, 106
master - mean (99ms) : 96, 103
section Bailout
This PR (8785) - mean (101ms) : 97, 104
master - mean (103ms) : 100, 107
section CallTarget+Inlining+NGEN
This PR (8785) - mean (822ms) : 783, 860
master - mean (821ms) : 785, 858
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 (8785) - mean (200ms) : 194, 205
master - mean (201ms) : 193, 209
section Bailout
This PR (8785) - mean (205ms) : 200, 209
master - mean (205ms) : 197, 213
section CallTarget+Inlining+NGEN
This PR (8785) - mean (1,202ms) : 1162, 1242
master - mean (1,197ms) : 1163, 1231
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 (8785) - mean (285ms) : 278, 292
master - mean (287ms) : 281, 293
section Bailout
This PR (8785) - mean (287ms) : 281, 292
master - mean (285ms) : 279, 291
section CallTarget+Inlining+NGEN
This PR (8785) - mean (964ms) : 941, 988
master - mean (964ms) : 945, 984
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8785) - mean (283ms) : 275, 290
master - mean (282ms) : 274, 289
section Bailout
This PR (8785) - mean (282ms) : 273, 290
master - mean (282ms) : 275, 289
section CallTarget+Inlining+NGEN
This PR (8785) - mean (1,160ms) : 1131, 1189
master - mean (1,161ms) : 1126, 1197
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8785) - mean (278ms) : 272, 283
master - mean (281ms) : 273, 289
section Bailout
This PR (8785) - mean (276ms) : 270, 281
master - mean (282ms) : 276, 288
section CallTarget+Inlining+NGEN
This PR (8785) - mean (1,039ms) : 1001, 1078
master - mean (1,038ms) : 1007, 1070
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionParserHelper.cs:90
ProbeExpressionParserHelper.Methodsintends to cache reflection lookups, but the key type (ReflectionMethodIdentifier) storesType[]arrays. As a record struct, its generated equality compares those arrays by reference, so most call sites that pass freshly-allocatednew[] { ... }/[...]arrays will never hit the cache and will continuously add new entries (unbounded growth + extra reflection work).
internal readonly record struct ReflectionMethodIdentifier
{
internal ReflectionMethodIdentifier(Type type, string methodName, Type[] parameters, Type[] genericArguments)
{
Type = type;
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8569d733b5
ℹ️ 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".
Contributor
Author
|
For reviewer context, here is the current Key details that changed from the earlier version:
flowchart TD
A["User writes instanceof expression"] --> B["Probe expression parser"]
B --> C["Emit runtime helper call"]
C --> D["InstanceOfHelper.IsInstanceOf"]
D --> E["ResolveType"]
E --> F{"Empty or null type name?"}
F -->|"Yes"| F1["Evaluation error: failed to parse type name"]
F -->|"No"| G{"Known BCL/core alias or exact known System type?"}
G -->|"Yes"| G1["Return known Type immediately; no assembly scan"]
G -->|"No"| H{"Cache hit?"}
H -->|"Resolved Type alive"| H1["Return cached Type"]
H -->|"Resolved Type was collected"| H2["Discard stale cache entry"]
H -->|"Cached miss for current generation"| H3["Evaluation error: unknown type; no new assemblies loaded"]
H -->|"Ambiguous"| H4["Evaluation error: ambiguous type"]
H -->|"No usable cache entry"| I["Parse type name"]
H2 --> I
I --> J{"Top-level assembly name provided?"}
J -->|"Yes"| K["AssemblyQualifiedTypeResolver"]
K --> K1{"Known framework assembly/name?"}
K1 -->|"Yes"| K2["Resolve through framework path; generic args may use framework or already-loaded exact assemblies"]
K1 -->|"No"| K3["Require fully qualified type name, then scan loaded matching assemblies"]
J -->|"No"| L{"Simple System type and no assembly-qualified generic args?"}
L -->|"Yes"| L1["Type.GetType fast path"]
L1 -->|"Found"| L2["Weak-cache Type and return"]
L1 -->|"Not found"| M{"Contains namespace dot?"}
L -->|"No"| M
M -->|"No"| M1["Evaluation error: must be fully qualified"]
M -->|"Yes"| N["Scan currently loaded assemblies"]
K3 --> O["For each eligible assembly"]
N --> O
flowchart TD
O["For each eligible assembly"] --> O1["Skip assemblies already scanned for this exact lookup when safe"]
O1 --> O2{"Top-level assembly name requested?"}
O2 -->|"Yes, and assembly identity does not match"| S["No match in this assembly"]
O2 -->|"No, or identity matches"| P{"Type name shape"}
P -->|"Simple type name"| Q["Exact case-sensitive Assembly.GetType"]
Q -->|"Found"| R1["Candidate Type"]
Q -->|"Not found"| S
P -->|"Constructed generic with assembly-qualified args"| T["Type.GetType with resolver callbacks"]
T --> U["Assembly resolver returns known framework assembly or already-loaded exact assembly"]
U --> V["Simple-name fallback only for simple-name requests"]
V --> W["No assembly loading side effects"]
W -->|"Found"| R1
W -->|"Not found"| S
R1 --> X{"Scan result after all eligible assemblies"}
S --> X
X -->|"No match"| X1["Cache scanned assemblies and miss for observed generation"]
X1 --> X2{"Assembly loaded during scan?"}
X2 -->|"Yes"| X3["Retry bounded scan"]
X2 -->|"No"| X4["Evaluation error: unknown type"]
X -->|"One match"| X5["Cache weak Type reference and scanned assembly identities"]
X5 --> X6["Return Type"]
X -->|"Multiple different matches"| X7["Cache ambiguous result"]
X7 --> X8["Evaluation error: ambiguous type"]
flowchart TD
A["Resolved Type returned to IsInstanceOf"] --> B{"Value category"}
B -->|"Reference type or nullable value type"| C["Use Type.IsInstanceOfType"]
B -->|"Non-nullable value type"| D["Use Type.IsAssignableFrom(typeof(TValue))"]
C --> E["If false, optionally debug-log suspicious same-full-name assembly mismatch"]
D --> E
E --> F["Return bool to expression evaluator"]
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5963a2235
ℹ️ 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".
jpbempel
reviewed
Jun 16, 2026
dudikeleti
force-pushed
the
dudik/el-instanceof-runtime-resolver
branch
from
June 17, 2026 13:44
f5963a2 to
373fbb1
Compare
Keep the common FQN resolution path focused while isolating assembly-qualified handling, and replace scanned assembly identity strings with value-type snapshots.
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
Co-authored-by: Cursor <[email protected]>
dudikeleti
force-pushed
the
dudik/el-instanceof-runtime-resolver
branch
from
June 17, 2026 16:18
373fbb1 to
97b22a3
Compare
jpbempel
approved these changes
Jun 17, 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
instanceofevaluation to resolve target types at runtime instead of parse/compile time.InstanceOfHelperto resolve known framework types and customer types without intentionally loading additional customer assemblies.int,string,Guid,DateTime,TimeSpan,Type,Exception,Array, and relatedSystem.*names.Reason for change
instanceofpreviously resolved target types too early, which meant expressions could fail when customer types were loaded after probe parsing.Implementation details
instanceofnow compiles to a runtime helper call:InstanceOfHelper.IsInstanceOf<TValue>(value, typeName).AppDomain.CurrentDomain.GetAssemblies()only when an expression is evaluated.Typereference remains alive.Assembly.GetType; there is no case-insensitive fallback for customer types.Type.GetTyperesolution where assembly callbacks return only known framework assemblies or already-loaded assemblies. Missing generic argument assemblies stay unresolved instead of being loaded.Assemblyreferences.Typevalues are cached throughWeakReference<Type>so the cache does not keep collectible AssemblyLoadContexts alive. If a weak type target is collected, that cache entry is discarded and the next lookup resolves from the currently loaded assemblies again.Test coverage
DebuggerExpressionLanguageTestscoverage for runtimeinstanceofresolution across conditions, templates, capture expressions, span decorations, known BCL/framework names, customer names, assembly-qualified names, generic arguments, ambiguity, cache behavior, lazy assembly loading, no-load behavior, exact matching, and bounded retry behavior.instanceofexpression output and unknown-type error behavior.Other details