[Native] Make GetTypeInfo Iterative to Prevent Native Stack Exhaustion#8708
Merged
Conversation
APMS-19659: extend the depth limit from PR #4415 to type_extends, nested types, and GetSigTypeTokName so ReJIT TypeDef enumeration cannot exhaust the 32-bit IIS worker thread stack.
This comment has been minimized.
This comment has been minimized.
![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: 77aea0c7a8
ℹ️ 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".
BenchmarksBenchmark execution time: 2026-05-29 16:41:02 Comparing candidate commit 491dad5 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 62 known flaky benchmarks, 64 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8708) 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 (8708) - mean (74ms) : 71, 78
master - mean (73ms) : 71, 75
section Bailout
This PR (8708) - mean (80ms) : 76, 84
master - mean (79ms) : 74, 83
section CallTarget+Inlining+NGEN
This PR (8708) - mean (1,113ms) : 1059, 1166
master - mean (1,114ms) : 1067, 1161
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 (8708) - mean (119ms) : 113, 124
master - mean (116ms) : 111, 120
section Bailout
This PR (8708) - mean (117ms) : 113, 120
master - mean (118ms) : 113, 123
section CallTarget+Inlining+NGEN
This PR (8708) - mean (797ms) : 779, 815
master - mean (798ms) : 769, 827
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8708) - mean (105ms) : 99, 111
master - mean (102ms) : 97, 107
section Bailout
This PR (8708) - mean (103ms) : 99, 107
master - mean (106ms) : 102, 111
section CallTarget+Inlining+NGEN
This PR (8708) - mean (956ms) : 925, 988
master - mean (959ms) : 912, 1005
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8708) - mean (102ms) : 97, 107
master - mean (100ms) : 97, 104
section Bailout
This PR (8708) - mean (104ms) : 99, 109
master - mean (104ms) : 98, 110
section CallTarget+Inlining+NGEN
This PR (8708) - mean (829ms) : 789, 870
master - mean (831ms) : 785, 877
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 (8708) - mean (199ms) : 192, 205
master - mean (198ms) : 193, 204
section Bailout
This PR (8708) - mean (202ms) : 197, 207
master - mean (201ms) : 196, 206
section CallTarget+Inlining+NGEN
This PR (8708) - mean (1,193ms) : 1151, 1235
master - mean (1,196ms) : 1147, 1246
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 (8708) - mean (284ms) : 277, 290
master - mean (285ms) : 277, 292
section Bailout
This PR (8708) - mean (285ms) : 278, 293
master - mean (284ms) : 276, 293
section CallTarget+Inlining+NGEN
This PR (8708) - mean (960ms) : 941, 979
master - mean (962ms) : 945, 979
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8708) - mean (275ms) : 268, 282
master - mean (277ms) : 269, 285
section Bailout
This PR (8708) - mean (276ms) : 270, 282
master - mean (277ms) : 270, 283
section CallTarget+Inlining+NGEN
This PR (8708) - mean (1,157ms) : 1121, 1192
master - mean (1,162ms) : 1132, 1192
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8708) - mean (276ms) : 271, 282
master - mean (277ms) : 272, 283
section Bailout
This PR (8708) - mean (277ms) : 272, 282
master - mean (276ms) : 270, 282
section CallTarget+Inlining+NGEN
This PR (8708) - mean (1,036ms) : 999, 1073
master - mean (1,036ms) : 989, 1084
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewlock
added
type:bug
area:native-library
andrewlock
reviewed
May 29, 2026
link04
changed the title
link04
enabled auto-merge (squash)
May 29, 2026 17:09
Collaborator
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
tonyredondo
approved these changes
Jun 5, 2026
tonyredondo
left a comment
tonyredondo
left a comment
Member
There was a problem hiding this comment.
seems good, maybe we could improve testing, but I guess is a general issue for all our native code.
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
4 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
Fixes a native stack exhaustion in
Datadog.Tracer.Native.dllon 32-bit IIS worker processes.PR #4415 added a recursion depth cap for
mdtTypeSpec/ELEMENT_TYPE_GENERICINSTcycles inGetTypeInfo, but the other recursive paths were still uncapped:type_extends, which walks the inheritance chainparent_type_token, which walks the nested class hierarchyDuring ReJIT,
rejit_preprocessorenumerates everyTypeDefin an assembly and callsGetTypeInfofor Derived/Interface integrations. Customer assemblies with deeply chained non-generic inheritance (or deep nesting) can recurse far enough to use up the ~256 KB default thread stack on x86 (eachGetTypeInfoframe takes about 2 KB fortype_name), which faults the process.Instead of capping the depth, which would silently stop resolving legitimately deep hierarchies that used to work, this PR converts the recursion into an iterative, queue-based traversal. That removes the stack pressure without putting any limit on how deep we go.
Here's what changed:
ResolveTypeInfoLeaf, which reads a single token's immediate metadata (name, flags, directtype_extends, directparent_type_token) without recursing into base or enclosing types. When it unwraps a TypeSpec generic instantiation it only tracks the TypeSpecs seen on the current unwrap path, so sibling branches can't cause false cycle detection.GetTypeInfonow runs in three phases. First it does a breadth-first walk over astd::dequework queue, reading each reachable token's leaf exactly once and adding any newly found base or enclosing tokens to the queue. Then it builds oneTypeInfonode per token (valueTypecomes from the base leaf's name, which is already resolved). Finally it links each node'sextend_fromandparent_typeto the nodes it built.GetSigTypeTokNameis left exactly as it is onmaster. Its recursion is bounded by how deeply a signature is nested rather than by type chains, it now calls the iterativeGetTypeInfo, and its TypeSpec cycle was already handled by [BugFix] Break recursion in GetTypeInfo #4415.Testing
What we built locally (removed before push)
Samples.DeepTypeHierarchy/DeepNonGenericChains.csGetTypeInforecursionSamples.DeepTypeHierarchy/Samples.DeepTypeHierarchy.csprojclr_helper_deep_hierarchy_test.cppTypeDefs and callsGetTypeInfo(mirrorsrejit_preprocessor)Datadog.Tracer.Native.Tests.vcxprojchangesSamples.DeepTypeHierarchyandStackReserveSize=262144on x86 Release to match the IIS w3wp stackregression/DeepNestedHierarchy/DeepNonGenericChains.csWe first added the deep types to
Samples.ExampleLibrary, which broke 3 existing native tests (EnumeratesTypeDefs,GetsTypeInfoFromTypeDefs,GetsTypeInfoFromMethods) because their expected type lists no longer matched. Moving the types to a dedicated assembly fixed that, but it still added a lot of test infrastructure, so we dropped it for this PR.How we verified the fix: with that native gtest in place and the test executable forced to a 256 KB stack using
editbin /STACK:262144(the default test-host stack is around 1 MB and hides the problem), the deep-hierarchy test crashed with a stack overflow before the change and passed after it. The existing native suite (89 tests, x86 Release) also passes with the change.Follow-up (optional): land the
Samples.DeepTypeHierarchysample, the native gtest, and the x86StackReserveSize=262144in a separate PR so CI catches this automatically, the same way PR #4415 added TypeSpec coverage.Local validation (Windows)
VsDevCmd.bat)CompileNativeLoaderWindows)W3SVCrunning. We didn't use it for the final validation because the native metadata test hits the same code path more directly.cdb.exeavailable underC:\Program Files (x86)\Windows Kits\10\Debuggers\after the SDK install. Customer dumps aren't in the workspace, so nothing was symbolicated in this PR.