[BugFix] Break recursion in GetTypeInfo by dromanol · Pull Request #4415 · DataDog/dd-trace-dotnet

dromanol · 2023-07-18T19:10:00Z

Summary of changes

Transform GetTypeInfo function in native library from recursive to iterative.
Fixed unwanted default activation of ASM instrumentations.
Improved logs.

Reason for change

In Alpine a stack overflow will arise when certain TypeSpec was requested when scanning the Microsoft.AspNetCore.OData assembly due to an unwanted Derived kind CallTarget instrumentation.

Implementation details

Removed GetTypeInfo recursion.
Fixed ASM CallTarget instrumentations toggle condition.

Test coverage

New native test added to ensure GetTypeInfo works fine when asked for MethodSpec info
New smoke test added

datadog-ddstaging · 2023-07-18T19:33:49Z

Datadog Report

Branch report: dani/apm/gettypeinfo_crash_fix
Commit report: a152229

❄️ dd-trace-dotnet: 0 Failed, 27 New Flaky, 300967 Passed, 2133 Skipped, 26m 13.88s Wall Time

New Flaky Tests (27)

TestMergeWithWaf - Datadog.Trace.Security.Unit.Tests.WafIpBlockTests - Last Failure

Expand for error

 Expected the enum to be ReturnCode.Match {value: 1}, but found ReturnCode.ErrorInternal {value: -4}.

TestOk - Datadog.Trace.Security.Unit.Tests.WafIpBlockTests - Last Failure

Expand for error

 Expected the enum to be ReturnCode.Match {value: 1}, but found ReturnCode.ErrorInternal {value: -4}.

AttacksWithSecrets - Datadog.Trace.Security.Unit.Tests.WafObfuscationTests - Last Failure

Expand for error

 Expected the enum to be ReturnCode.Match {value: 1}, but found ReturnCode.ErrorInternal {value: -4}.

AttacksWithSecrets - Datadog.Trace.Security.Unit.Tests.WafObfuscationTests - Last Failure

Expand for error

 Expected the enum to be ReturnCode.Match {value: 1}, but found ReturnCode.ErrorInternal {value: -4}.

AttacksWithSecrets - Datadog.Trace.Security.Unit.Tests.WafObfuscationTests - Last Failure

Expand for error

 Expected the enum to be ReturnCode.Match {value: 1}, but found ReturnCode.ErrorInternal {value: -4}.

tonyredondo · 2023-07-20T09:48:41Z

+                if (signature[0] & ELEMENT_TYPE_GENERICINST)
+                {
+                    mdToken type_token;
+                    CorSigUncompressToken(&signature[2], &type_token);
+                    token = type_token;
+                    continue;


There's a behaviour change here. We already talked about it offline.

Fixed in 961ba21

tonyredondo

LGTM, thanks for the fix

andrewlock · 2023-07-20T11:03:04Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-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 shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

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).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4415) - mean (3,000ms)  : 2948, 3051
     .   : milestone, 3000,
    master - mean (2,997ms)  : 2930, 3063
     .   : milestone, 2997,

    section CallTarget+Inlining+NGEN
    This PR (4415) - mean (3,822ms)  : 3732, 3911
     .   : milestone, 3822,
    master - mean (3,801ms)  : 3724, 3877
     .   : milestone, 3801,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4415) - mean (3,131ms)  : 3069, 3193
     .   : milestone, 3131,
    master - mean (3,125ms)  : 3015, 3234
     .   : milestone, 3125,

    section CallTarget+Inlining+NGEN
    This PR (4415) - mean (3,633ms)  : 3558, 3708
     .   : milestone, 3633,
    master - mean (3,605ms)  : 3548, 3662
     .   : milestone, 3605,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4415) - mean (3,070ms)  : 2974, 3167
     .   : milestone, 3070,
    master - mean (3,087ms)  : 3013, 3161
     .   : milestone, 3087,

    section CallTarget+Inlining+NGEN
    This PR (4415) - mean (3,575ms)  : 3506, 3644
     .   : milestone, 3575,
    master - mean (3,571ms)  : 3511, 3632
     .   : milestone, 3571,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4415) - mean (190ms)  : 187, 193
     .   : milestone, 190,
    master - mean (190ms)  : 187, 194
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (4415) - mean (1,114ms)  : 1093, 1136
     .   : milestone, 1114,
    master - mean (1,118ms)  : 1092, 1143
     .   : milestone, 1118,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4415) - mean (372ms)  : 366, 377
     .   : milestone, 372,
    master - mean (373ms)  : 367, 379
     .   : milestone, 373,

    section CallTarget+Inlining+NGEN
    This PR (4415) - mean (1,170ms)  : 1147, 1194
     .   : milestone, 1170,
    master - mean (1,180ms)  : 1145, 1215
     .   : milestone, 1180,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4415) - mean (361ms)  : 356, 366
     .   : milestone, 361,
    master - mean (360ms)  : 352, 369
     .   : milestone, 360,

    section CallTarget+Inlining+NGEN
    This PR (4415) - mean (1,137ms)  : 1114, 1160
     .   : milestone, 1137,
    master - mean (1,132ms)  : 1104, 1161
     .   : milestone, 1132,

NachoEchevarria

LGTM

andrewlock · 2023-07-20T12:12:00Z

Benchmarks Report 🐌

Benchmarks for #4415 compared to master:

2 benchmarks are faster, with geometric mean 1.158
1 benchmarks are slower, with geometric mean 1.181
1 benchmarks have fewer allocations

The following thresholds were used for comparing the benchmark speeds:

Mann–Whitney U test with statistical test for significance of 5%
Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	470μs	254ns	949ns	0	2.62 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	655μs	203ns	786ns	0	2.62 KB
master	`WriteAndFlushEnrichedTraces`	net472	790μs	460ns	1.72μs	0.393	3.22 KB
#4415	`WriteAndFlushEnrichedTraces`	net6.0	483μs	769ns	2.98μs	0	2.62 KB
#4415	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	663μs	216ns	778ns	0	2.62 KB
#4415	`WriteAndFlushEnrichedTraces`	net472	795μs	300ns	1.12μs	0.396	3.22 KB

Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	37.5μs	89.4ns	346ns	0.0185	0	1.69 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	39.9μs	90ns	312ns	0.0198	0	1.67 KB
master	`AllCycleSimpleBody`	net472	42.8μs	110ns	411ns	0.258	0	1.73 KB
master	`AllCycleMoreComplexBody`	net6.0	216μs	63.3ns	245ns	0.108	0	9.26 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	229μs	71.5ns	258ns	0.115	0	9.16 KB
master	`AllCycleMoreComplexBody`	net472	235μs	51.2ns	192ns	1.41	0	9.33 KB
master	`ObjectExtractorSimpleBody`	net6.0	125ns	0.0316ns	0.118ns	0.00393	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	176ns	0.153ns	0.572ns	0.00374	0	272 B
master	`ObjectExtractorSimpleBody`	net472	153ns	0.102ns	0.396ns	0.0446	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	2.95μs	0.815ns	3.05ns	0.0545	0	3.88 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	4.04μs	1.59ns	5.96ns	0.0503	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	net472	4.18μs	3.63ns	14ns	0.617	0.00627	3.89 KB
#4415	`AllCycleSimpleBody`	net6.0	37.9μs	40.6ns	147ns	0.0195	0	1.69 KB
#4415	`AllCycleSimpleBody`	netcoreapp3.1	39.7μs	74ns	287ns	0.0198	0	1.67 KB
#4415	`AllCycleSimpleBody`	net472	41.4μs	18.3ns	63.5ns	0.27	0	1.73 KB
#4415	`AllCycleMoreComplexBody`	net6.0	222μs	67.6ns	253ns	0.109	0	9.26 KB
#4415	`AllCycleMoreComplexBody`	netcoreapp3.1	228μs	109ns	407ns	0.114	0	9.16 KB
#4415	`AllCycleMoreComplexBody`	net472	233μs	64.9ns	251ns	1.4	0	9.33 KB
#4415	`ObjectExtractorSimpleBody`	net6.0	124ns	0.0341ns	0.128ns	0.00396	0	280 B
#4415	`ObjectExtractorSimpleBody`	netcoreapp3.1	176ns	0.103ns	0.387ns	0.00374	0	272 B
#4415	`ObjectExtractorSimpleBody`	net472	144ns	0.0942ns	0.365ns	0.0446	0	281 B
#4415	`ObjectExtractorMoreComplexBody`	net6.0	2.99μs	1.21ns	4.68ns	0.055	0	3.88 KB
#4415	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	4.07μs	2.45ns	9.18ns	0.0507	0	3.78 KB
#4415	`ObjectExtractorMoreComplexBody`	net472	4.19μs	3.62ns	14ns	0.618	0.00627	3.89 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net6.0	169μs	114ns	425ns	0.252	18.08 KB
master	`SendRequest`	netcoreapp3.1	189μs	195ns	754ns	0.188	20.14 KB
master	`SendRequest`	net472	0.000132ns	9.64E‑05ns	0.000348ns	0	0 b
#4415	`SendRequest`	net6.0	170μs	187ns	725ns	0.168	18.08 KB
#4415	`SendRequest`	netcoreapp3.1	186μs	225ns	870ns	0.188	20.14 KB
#4415	`SendRequest`	net472	0.00165ns	0.000635ns	0.00238ns	0	0 b

Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #4415

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1	41.94 KB	41.63 KB	-318 B	-0.76%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	519μs	504ns	1.95μs	0.512	0	0	41.58 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	629μs	1.05μs	4.09μs	0.308	0	0	41.94 KB
master	`WriteAndFlushEnrichedTraces`	net472	801μs	3.91μs	16.6μs	8.36	2.39	0.398	53.23 KB
#4415	`WriteAndFlushEnrichedTraces`	net6.0	522μs	371ns	1.34μs	0.519	0	0	41.71 KB
#4415	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	634μs	1.1μs	4.28μs	0.312	0	0	41.63 KB
#4415	`WriteAndFlushEnrichedTraces`	net472	781μs	3μs	11.2μs	8.31	2.37	0.396	53.23 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteNonQuery`	net6.0	983ns	0.237ns	0.919ns	0.0109	768 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.24μs	0.467ns	1.81ns	0.0106	768 B
master	`ExecuteNonQuery`	net472	1.49μs	0.538ns	2.08ns	0.116	730 B
#4415	`ExecuteNonQuery`	net6.0	907ns	0.369ns	1.38ns	0.0105	768 B
#4415	`ExecuteNonQuery`	netcoreapp3.1	1.22μs	0.483ns	1.81ns	0.0104	768 B
#4415	`ExecuteNonQuery`	net472	1.38μs	3.16ns	12.2ns	0.115	730 B

Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #4415

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑net6.0	1.181	1,037.04	1,224.75

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`CallElasticsearch`	net6.0	1.03μs	0.324ns	1.21ns	0.014	0	992 B
master	`CallElasticsearch`	netcoreapp3.1	1.35μs	0.29ns	1.09ns	0.0134	0	992 B
master	`CallElasticsearch`	net472	2.05μs	0.64ns	2.4ns	0.159	0.00103	1 KB
master	`CallElasticsearchAsync`	net6.0	1.04μs	0.384ns	1.44ns	0.0135	0	968 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.48μs	0.52ns	1.94ns	0.0139	0	1.04 KB
master	`CallElasticsearchAsync`	net472	2.33μs	0.626ns	2.26ns	0.168	0.00117	1.06 KB
#4415	`CallElasticsearch`	net6.0	1.09μs	0.442ns	1.65ns	0.0136	0	992 B
#4415	`CallElasticsearch`	netcoreapp3.1	1.33μs	0.278ns	1.04ns	0.0135	0	992 B
#4415	`CallElasticsearch`	net472	2.08μs	0.486ns	1.88ns	0.159	0.00104	1 KB
#4415	`CallElasticsearchAsync`	net6.0	1.22μs	0.356ns	1.33ns	0.0135	0	968 B
#4415	`CallElasticsearchAsync`	netcoreapp3.1	1.41μs	0.467ns	1.75ns	0.0141	0	1.04 KB
#4415	`CallElasticsearchAsync`	net472	2.21μs	0.538ns	2.01ns	0.168	0	1.06 KB

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteAsync`	net6.0	1.14μs	0.372ns	1.44ns	0.0124	0	912 B
master	`ExecuteAsync`	netcoreapp3.1	1.38μs	0.72ns	2.69ns	0.0122	0	912 B
master	`ExecuteAsync`	net472	1.52μs	0.494ns	1.85ns	0.139	0.000757	875 B
#4415	`ExecuteAsync`	net6.0	1.15μs	0.282ns	1.05ns	0.0126	0	912 B
#4415	`ExecuteAsync`	netcoreapp3.1	1.36μs	0.354ns	1.22ns	0.0122	0	912 B
#4415	`ExecuteAsync`	net472	1.57μs	0.493ns	1.91ns	0.138	0.000787	875 B

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net6.0	3.64μs	1.39ns	5.19ns	0.0273	1.94 KB
master	`SendAsync`	netcoreapp3.1	4.42μs	0.774ns	2.9ns	0.0329	2.48 KB
master	`SendAsync`	net472	6.92μs	2.31ns	8.63ns	0.481	3.05 KB
#4415	`SendAsync`	net6.0	3.52μs	1.11ns	4.14ns	0.0265	1.94 KB
#4415	`SendAsync`	netcoreapp3.1	4.33μs	1.12ns	4.18ns	0.0325	2.48 KB
#4415	`SendAsync`	net472	6.73μs	3.11ns	11.7ns	0.484	3.05 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.29μs	0.829ns	3.21ns	0.0224	1.62 KB
master	`EnrichedLog`	netcoreapp3.1	2.06μs	1.31ns	4.92ns	0.0217	1.62 KB
master	`EnrichedLog`	net472	2.27μs	2.92ns	10.9ns	0.244	1.54 KB
#4415	`EnrichedLog`	net6.0	1.33μs	0.737ns	2.76ns	0.0225	1.62 KB
#4415	`EnrichedLog`	netcoreapp3.1	1.96μs	1.15ns	4.44ns	0.0215	1.62 KB
#4415	`EnrichedLog`	net472	2.19μs	0.597ns	2.07ns	0.245	1.54 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net6.0	111μs	175ns	679ns	0.0561	0	4.21 KB
master	`EnrichedLog`	netcoreapp3.1	117μs	139ns	519ns	0	0	4.21 KB
master	`EnrichedLog`	net472	146μs	164ns	614ns	0.655	0.218	4.38 KB
#4415	`EnrichedLog`	net6.0	113μs	109ns	421ns	0.0565	0	4.21 KB
#4415	`EnrichedLog`	netcoreapp3.1	117μs	112ns	432ns	0	0	4.21 KB
#4415	`EnrichedLog`	net472	145μs	136ns	528ns	0.653	0.218	4.38 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.74μs	1.27ns	4.74ns	0.0299	2.18 KB
master	`EnrichedLog`	netcoreapp3.1	3.82μs	1.4ns	5.22ns	0.0286	2.18 KB
master	`EnrichedLog`	net472	4.42μs	1.72ns	6.65ns	0.314	1.99 KB
#4415	`EnrichedLog`	net6.0	2.93μs	1.19ns	4.47ns	0.0309	2.18 KB
#4415	`EnrichedLog`	netcoreapp3.1	3.82μs	1.98ns	7.67ns	0.0289	2.18 KB
#4415	`EnrichedLog`	net472	4.38μs	2.42ns	9.38ns	0.315	1.99 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendReceive`	net6.0	1.27μs	0.574ns	2.07ns	0.0164	0	1.16 KB
master	`SendReceive`	netcoreapp3.1	1.56μs	0.611ns	2.29ns	0.0156	0	1.16 KB
master	`SendReceive`	net472	1.78μs	2.84ns	11ns	0.184	0.000898	1.16 KB
#4415	`SendReceive`	net6.0	1.2μs	0.541ns	2.03ns	0.0161	0	1.16 KB
#4415	`SendReceive`	netcoreapp3.1	1.56μs	2.02ns	7.28ns	0.0154	0	1.16 KB
#4415	`SendReceive`	net472	1.86μs	0.473ns	1.77ns	0.185	0	1.16 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.5μs	0.927ns	3.59ns	0.0215	1.53 KB
master	`EnrichedLog`	netcoreapp3.1	3.52μs	1.72ns	6.44ns	0.0215	1.58 KB
master	`EnrichedLog`	net472	4.01μs	1.47ns	5.49ns	0.31	1.96 KB
#4415	`EnrichedLog`	net6.0	2.59μs	0.502ns	1.88ns	0.0208	1.53 KB
#4415	`EnrichedLog`	netcoreapp3.1	3.39μs	1.5ns	5.63ns	0.0204	1.58 KB
#4415	`EnrichedLog`	net472	4.02μs	1.9ns	7.1ns	0.311	1.96 KB

Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #4415

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0	1.164	498.67	428.34
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.151	517.91	449.83

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	499ns	0.198ns	0.743ns	0.00744	536 B
master	`StartFinishSpan`	netcoreapp3.1	530ns	0.261ns	0.976ns	0.00737	536 B
master	`StartFinishSpan`	net472	614ns	0.486ns	1.88ns	0.0854	538 B
master	`StartFinishScope`	net6.0	518ns	0.191ns	0.714ns	0.00931	656 B
master	`StartFinishScope`	netcoreapp3.1	767ns	0.218ns	0.846ns	0.00885	656 B
master	`StartFinishScope`	net472	771ns	0.497ns	1.93ns	0.0981	618 B
#4415	`StartFinishSpan`	net6.0	428ns	0.134ns	0.502ns	0.00758	536 B
#4415	`StartFinishSpan`	netcoreapp3.1	527ns	0.278ns	1.08ns	0.00726	536 B
#4415	`StartFinishSpan`	net472	624ns	0.241ns	0.901ns	0.0854	538 B
#4415	`StartFinishScope`	net6.0	450ns	0.109ns	0.408ns	0.00922	656 B
#4415	`StartFinishScope`	netcoreapp3.1	764ns	0.273ns	0.986ns	0.0089	656 B
#4415	`StartFinishScope`	net472	809ns	1.06ns	3.97ns	0.098	618 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	562ns	0.0877ns	0.304ns	0.00933	656 B
master	`RunOnMethodBegin`	netcoreapp3.1	787ns	0.26ns	1.01ns	0.00893	656 B
master	`RunOnMethodBegin`	net472	928ns	0.79ns	2.96ns	0.098	618 B
#4415	`RunOnMethodBegin`	net6.0	512ns	0.462ns	1.73ns	0.00919	656 B
#4415	`RunOnMethodBegin`	netcoreapp3.1	763ns	0.195ns	0.703ns	0.0091	656 B
#4415	`RunOnMethodBegin`	net472	907ns	0.151ns	0.544ns	0.0981	618 B

anna-git · 2023-07-20T12:33:18Z

did you mean to include this file in the PR?

Clearly not :_

andrewlock · 2023-07-20T12:45:00Z

Throughput/Crank Report:zap:

Throughput results for AspNetCoreSimpleController comparing the following branches/commits:

Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.

Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!

gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4415) (11.115M)   : 0, 11114822
    master (11.200M)   : 0, 11200305
    benchmarks/2.33.0 (11.022M)   : 0, 11022348
    benchmarks/2.9.0 (11.272M)   : 0, 11272042

    section Automatic
    This PR (4415) (7.829M)   : 0, 7828536
    master (7.774M)   : 0, 7773579
    benchmarks/2.33.0 (7.685M)   : 0, 7684993
    benchmarks/2.9.0 (8.192M)   : 0, 8191645

    section Trace stats
    master (7.757M)   : 0, 7756541
    benchmarks/2.33.0 (7.732M)   : 0, 7731884

    section Manual
    This PR (4415) (10.071M)   : 0, 10071441
    master (10.129M)   : 0, 10129211
    benchmarks/2.33.0 (10.053M)   : 0, 10053358

    section Manual + Automatic
    This PR (4415) (7.715M)   : 0, 7715310
    master (7.448M)   : 0, 7448326
    benchmarks/2.33.0 (7.522M)   : 0, 7522014

    section Version Conflict
    master (6.737M)   : 0, 6737238
    benchmarks/2.33.0 (6.743M)   : 0, 6742736

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4415) (9.475M)   : 0, 9474923
    master (9.687M)   : 0, 9686595
    benchmarks/2.33.0 (9.663M)   : 0, 9662630
    benchmarks/2.9.0 (9.466M)   : 0, 9465786

    section Automatic
    This PR (4415) (6.837M)   : 0, 6837345
    master (6.783M)   : 0, 6782784
    benchmarks/2.33.0 (6.697M)   : 0, 6697224

    section Trace stats
    master (6.499M)   : 0, 6498829
    benchmarks/2.33.0 (6.582M)   : 0, 6581611

    section Manual
    This PR (4415) (8.651M)   : 0, 8650595
    master (8.523M)   : 0, 8523488
    benchmarks/2.33.0 (8.648M)   : 0, 8648416

    section Manual + Automatic
    This PR (4415) (6.372M)   : 0, 6372280
    master (6.416M)   : 0, 6415934
    benchmarks/2.33.0 (6.435M)   : 0, 6434634

    section Version Conflict
    master (5.950M)   : 0, 5949585
    benchmarks/2.33.0 (5.780M)   : 0, 5780078

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4415) (10.148M)   : 0, 10147830
    master (10.142M)   : 0, 10142225
    benchmarks/2.33.0 (10.194M)   : 0, 10194072
    benchmarks/2.9.0 (9.296M)   : 0, 9295675

    section Automatic
    This PR (4415) (7.241M)   : 0, 7241479
    master (7.252M)   : 0, 7252121
    benchmarks/2.33.0 (7.394M)   : 0, 7393957
    benchmarks/2.9.0 (6.971M)   : 0, 6971028

    section Trace stats
    master (7.227M)   : 0, 7226510
    benchmarks/2.33.0 (7.436M)   : 0, 7435717

    section Manual
    This PR (4415) (9.097M)   : 0, 9097165
    master (9.085M)   : 0, 9085482
    benchmarks/2.33.0 (9.450M)   : 0, 9450071

    section Manual + Automatic
    This PR (4415) (7.194M)   : 0, 7193562
    master (6.873M)   : 0, 6873367
    benchmarks/2.33.0 (7.110M)   : 0, 7109611

    section Version Conflict
    master (6.372M)   : 0, 6372363
    benchmarks/2.33.0 (6.592M)   : 0, 6592321

gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4415) (7.362M)   : 0, 7362491
    master (7.104M)   : 0, 7104363
    benchmarks/2.33.0 (7.453M)   : 0, 7453377
    benchmarks/2.9.0 (7.919M)   : 0, 7919483

    section No attack
    This PR (4415) (2.072M)   : 0, 2071770
    master (2.133M)   : 0, 2133338
    benchmarks/2.33.0 (2.138M)   : 0, 2138287
    benchmarks/2.9.0 (3.252M)   : 0, 3251832

    section Attack
    This PR (4415) (1.633M)   : 0, 1633171
    master (1.627M)   : 0, 1627257
    benchmarks/2.33.0 (1.867M)   : 0, 1866625
    benchmarks/2.9.0 (2.551M)   : 0, 2550900

    section Blocking
    This PR (4415) (3.052M)   : 0, 3052426
    master (3.104M)   : 0, 3104368
    benchmarks/2.33.0 (3.720M)   : 0, 3719806

* Break recursion in GetTypeInfo * Algo fix and test added * Fixed c++ warning * Fix ASM instrumentations being enabled when they should not * Improved RegisterCallTarget traces * Add smoke tests for deeply nested hierarchies * Add net462 support to DeepNestedHierachy test * Minor fix and perf improvement --------- Co-authored-by: Andrew Lock <[email protected]>

#8708) ## Summary Fixes a native stack exhaustion in `Datadog.Tracer.Native.dll` on **32-bit IIS worker processes**. PR #4415 added a recursion depth cap for `mdtTypeSpec` / `ELEMENT_TYPE_GENERICINST` cycles in `GetTypeInfo`, but the other recursive paths were still uncapped: - **`type_extends`**, which walks the inheritance chain - **`parent_type_token`**, which walks the nested class hierarchy During ReJIT, `rejit_preprocessor` enumerates every `TypeDef` in an assembly and calls `GetTypeInfo` for 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 (each `GetTypeInfo` frame takes about 2 KB for `type_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: - Added `ResolveTypeInfoLeaf`, which reads a single token's immediate metadata (name, flags, direct `type_extends`, direct `parent_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. - `GetTypeInfo` now runs in three phases. First it does a breadth-first walk over a `std::deque` work queue, reading each reachable token's leaf exactly once and adding any newly found base or enclosing tokens to the queue. Then it builds one `TypeInfo` node per token (`valueType` comes from the base leaf's name, which is already resolved). Finally it links each node's `extend_from` and `parent_type` to the nodes it built. - Since each token is resolved at most once, malformed cyclic metadata can no longer loop forever. Before this, it would have overflowed the stack. - `GetSigTypeTokName` is left exactly as it is on `master`. Its recursion is bounded by how deeply a signature is nested rather than by type chains, it now calls the iterative `GetTypeInfo`, and its TypeSpec cycle was already handled by #4415. ## Testing ### What we built locally (removed before push) | File / area | Purpose | Why removed | |-------------|---------|-------------| | `Samples.DeepTypeHierarchy/DeepNonGenericChains.cs` | 100-level non-generic inheritance chain plus 50-level nested classes to trigger `GetTypeInfo` recursion | Would add ~150 new types and a new test assembly to the repo | | `Samples.DeepTypeHierarchy/Samples.DeepTypeHierarchy.csproj` | Builds the above as metadata input for native tests | Same, scope beyond the fix | | `clr_helper_deep_hierarchy_test.cpp` | Native gtest that loads that DLL, enumerates all `TypeDef`s and calls `GetTypeInfo` (mirrors `rejit_preprocessor`) | Depends on the test assembly above | | `Datadog.Tracer.Native.Tests.vcxproj` changes | Project ref to `Samples.DeepTypeHierarchy` and `StackReserveSize=262144` on x86 Release to match the IIS w3wp stack | Only needed for the automated regression | | `regression/DeepNestedHierarchy/DeepNonGenericChains.cs` | Managed smoke-test sample for the IIS/profiler path | Blocked on full monitoring-home setup, and the native metadata test was enough to validate the root cause | We 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.DeepTypeHierarchy` sample, the native gtest, and the x86 `StackReserveSize=262144` in a separate PR so CI catches this automatically, the same way PR #4415 added TypeSpec coverage. ## Local validation (Windows) - **VS 2022 Preview**, required for native builds (`VsDevCmd.bat`) - **Windows SDK 10.0.19041**, installed via winget (unblocked `CompileNativeLoaderWindows`) - **IIS**, `W3SVC` running. We didn't use it for the final validation because the native metadata test hits the same code path more directly. - **Debugging Tools**, `cdb.exe` available under `C:\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.

Break recursion in GetTypeInfo

b187a90

This comment has been minimized.

Sign in to view

Fix condition

f7791bb

This comment has been minimized.

Sign in to view

Algo fix and test added

46c466c

This comment has been minimized.

Sign in to view

Fixed c++ warning

bee5664

This comment has been minimized.

Sign in to view

dromanol added 3 commits July 19, 2023 12:42

Fix ASM instrumentations being enabled when they should not

07fa873

Improve call target traces

c7faa8f

Improved RegisterCallTarget traces

eecdb93

dromanol changed the title ~~Break recursion in GetTypeInfo~~ [BugFix] Break recursion in GetTypeInfo Jul 19, 2023

dromanol added type:bug area:asm labels Jul 19, 2023

This comment has been minimized.

Sign in to view

Add smoke tests for deeply nested hierarchies

46a964c

This comment has been minimized.

Sign in to view

dromanol marked this pull request as ready for review July 20, 2023 08:21

dromanol requested review from a team as code owners July 20, 2023 08:21

Add net462 support to DeepNestedHierachy test

7b9e3cb

This comment has been minimized.

Sign in to view

anna-git reviewed Jul 20, 2023

View reviewed changes

Comment thread tracer/src/Datadog.Trace/ClrProfiler/Instrumentation.cs

This comment has been minimized.

Sign in to view

tonyredondo reviewed Jul 20, 2023

View reviewed changes

Minor fix and perf improvement

961ba21

dromanol requested a review from a team as a code owner July 20, 2023 09:59

Subtle fix

a152229

tonyredondo approved these changes Jul 20, 2023

View reviewed changes

NachoEchevarria approved these changes Jul 20, 2023

View reviewed changes

anna-git reviewed Jul 20, 2023

View reviewed changes

dromanol merged commit c64aef8 into master Jul 20, 2023

dromanol deleted the dani/apm/gettypeinfo_crash_fix branch July 20, 2023 15:54

github-actions Bot added this to the vNext milestone Jul 20, 2023

andrewlock mentioned this pull request Jul 27, 2023

Adding a reference to Microsoft.AspNetCore.OData throws "Internal CLR Error" on .NET 6.0 and "Stack overflow" on .NET 7.0 #4393

Closed

link04 mentioned this pull request May 28, 2026

[Native] Make GetTypeInfo Iterative to Prevent Native Stack Exhaustion #8708

Merged

Uh oh!

Conversation

dromanol commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Uh oh!

datadog-ddstaging Bot commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

New Flaky Tests (27)

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

This comment has been minimized.

tonyredondo Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

dromanol Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

tonyredondo left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Jul 20, 2023

Execution-Time Benchmarks Report ⏱️

Uh oh!

NachoEchevarria left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Jul 20, 2023

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Fewer allocations 🎉 in #4415

Raw results

Raw results

Slower ⚠️ in #4415

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #4415

Raw results

Raw results

Uh oh!

anna-git Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

dromanol Jul 21, 2023

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Jul 20, 2023

Throughput/Crank Report:zap:

Uh oh!

dromanol commented Jul 18, 2023 •

edited

Loading

datadog-ddstaging Bot commented Jul 18, 2023 •

edited

Loading