Remove redundant string allocations by shurivich · Pull Request #3169 · DataDog/dd-trace-dotnet

shurivich · 2022-09-05T12:20:50Z

Summary of changes

Remove redundant string allocations.
Remove Encoding.UTF8.GetString and JsonSerializer with streams

Reason for change

Second part of removing redundant string allocations

OmerRaviv · 2022-09-07T08:02:59Z

            }

-            var contentDecode = Encoding.UTF8.GetString(Convert.FromBase64String((string)reader.Value));
+            var contentDecode = Convert.FromBase64String((string)reader.Value);


I'm not sure about this, but would using an intermediary CryptoStream as suggested on https://stackoverflow.com/a/35795205 instead of calling FromBase64String save us this allocation (which in the case of ASM's 240kb payload, will end up on the LOH)?

Nice.
Changed here 90affbc

Actually, it fails now on AssemblyReferencesHaveNotChanged test, so I'm reverting commit 😢

OmerRaviv

Really cool improvements, thanks!

This reverts commit 90affbc.

andrewlock · 2022-09-08T13:47:54Z

Benchmarks Report 🐌

Benchmarks for #3169 compared to master:

1 benchmarks are faster, with geometric mean 1.115
1 benchmarks are slower, with geometric mean 1.128
All benchmarks have the same 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`	net472	728μs	233ns	903ns	0.363	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	465μs	300ns	1.16μs	0	2.59 KB
#3169	`WriteAndFlushEnrichedTraces`	net472	723μs	438ns	1.64μs	0.361	3.18 KB
#3169	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	455μs	241ns	870ns	0	2.58 KB

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

Slower ⚠️ in #3169

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net472	1.128	205.34	231.57

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	185ns	0.17ns	0.635ns	0.0676	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	235ns	0.371ns	1.44ns	0.00577	0	424 B
master	`AllCycleMoreComplexBody`	net472	205ns	0.184ns	0.688ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	233ns	0.364ns	1.41ns	0.00549	0	400 B
master	`BodyExtractorSimpleBody`	net472	260ns	0.225ns	0.872ns	0.0573	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	219ns	0.379ns	1.42ns	0.00363	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.4μs	9.67ns	36.2ns	1.21	0.0216	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	14ns	52.5ns	0.0868	0	6.75 KB
#3169	`AllCycleSimpleBody`	net472	202ns	0.14ns	0.542ns	0.0676	0	425 B
#3169	`AllCycleSimpleBody`	netcoreapp3.1	241ns	0.241ns	0.933ns	0.00568	0	424 B
#3169	`AllCycleMoreComplexBody`	net472	231ns	0.134ns	0.501ns	0.0638	0	401 B
#3169	`AllCycleMoreComplexBody`	netcoreapp3.1	234ns	0.317ns	1.19ns	0.00557	0	400 B
#3169	`BodyExtractorSimpleBody`	net472	255ns	0.15ns	0.561ns	0.0573	0	361 B
#3169	`BodyExtractorSimpleBody`	netcoreapp3.1	215ns	0.282ns	1.05ns	0.00369	0	272 B
#3169	`BodyExtractorMoreComplexBody`	net472	14.8μs	5.43ns	20.3ns	1.21	0.0147	7.62 KB
#3169	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12μs	11.4ns	42.5ns	0.0897	0	6.75 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	182μs	177ns	687ns	0.182	20.57 KB
#3169	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3169	`SendRequest`	netcoreapp3.1	181μs	98ns	379ns	0.272	20.57 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.85μs	0.562ns	2.18ns	0.15	0.000921	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.41μs	0.54ns	2.02ns	0.0128	0	936 B
#3169	`ExecuteNonQuery`	net472	1.85μs	1.47ns	5.69ns	0.15	0.000927	947 B
#3169	`ExecuteNonQuery`	netcoreapp3.1	1.42μs	0.543ns	2.1ns	0.0127	0	936 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.64μs	2.1ns	7.85ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.47μs	1.05ns	4.05ns	0.0147	1.1 KB
master	`CallElasticsearchAsync`	net472	2.66μs	1.25ns	4.85ns	0.204	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	0.367ns	1.42ns	0.0164	1.22 KB
#3169	`CallElasticsearch`	net472	2.44μs	0.729ns	2.73ns	0.183	1.16 KB
#3169	`CallElasticsearch`	netcoreapp3.1	1.55μs	1.25ns	4.83ns	0.0148	1.1 KB
#3169	`CallElasticsearchAsync`	net472	2.58μs	1.53ns	5.94ns	0.205	1.29 KB
#3169	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	1.06ns	3.98ns	0.0162	1.22 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.64μs	5.43ns	21ns	0.226	1.42 KB
master	`ExecuteAsync`	netcoreapp3.1	1.72μs	3.42ns	13.3ns	0.0184	1.34 KB
#3169	`ExecuteAsync`	net472	2.59μs	8.99ns	34.8ns	0.225	1.42 KB
#3169	`ExecuteAsync`	netcoreapp3.1	1.69μs	4.65ns	18ns	0.0184	1.34 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.64μs	7.79ns	30.2ns	0.438	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.6μs	9.48ns	36.7ns	0.0357	2.6 KB
#3169	`SendAsync`	net472	5.73μs	17.9ns	67.1ns	0.44	2.77 KB
#3169	`SendAsync`	netcoreapp3.1	3.62μs	6.8ns	25.5ns	0.034	2.6 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.22μs	6.05ns	22.6ns	0.288	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.61μs	2.85ns	11ns	0.0246	1.85 KB
#3169	`EnrichedLog`	net472	3.29μs	2.4ns	9.3ns	0.287	1.81 KB
#3169	`EnrichedLog`	netcoreapp3.1	2.52μs	2.23ns	8.64ns	0.0252	1.85 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	87.6ns	339ns	0.683	0.228	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	116μs	143ns	535ns	0	0	4.49 KB
#3169	`EnrichedLog`	net472	153μs	155ns	579ns	0.689	0.23	4.65 KB
#3169	`EnrichedLog`	netcoreapp3.1	116μs	123ns	475ns	0.0582	0	4.49 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.43μs	14.7ns	57.1ns	0.569	0.00267	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.29μs	8.46ns	32.8ns	0.0534	0	3.91 KB
#3169	`EnrichedLog`	net472	5.57μs	14.2ns	54.8ns	0.568	0.00285	3.59 KB
#3169	`EnrichedLog`	netcoreapp3.1	4.5μs	9.18ns	34.3ns	0.0534	0	3.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.32μs	2.58ns	10ns	0.218	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.73μs	0.69ns	2.67ns	0.0173	1.32 KB
#3169	`SendReceive`	net472	2.34μs	1.75ns	6.8ns	0.218	1.37 KB
#3169	`SendReceive`	netcoreapp3.1	1.92μs	0.801ns	2.89ns	0.0173	1.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	4.98μs	2.27ns	8.81ns	0.352	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.22μs	1.33ns	4.97ns	0.0238	1.8 KB
#3169	`EnrichedLog`	net472	4.95μs	3.58ns	13.4ns	0.353	2.23 KB
#3169	`EnrichedLog`	netcoreapp3.1	4.49μs	1.88ns	7.26ns	0.0248	1.8 KB

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

Faster 🎉 in #3169

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1	1.115	1,154.70	1,035.43

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.18μs	0.638ns	2.39ns	0.129	810 B
master	`StartFinishSpan`	netcoreapp3.1	921ns	0.389ns	1.46ns	0.0104	760 B
master	`StartFinishScope`	net472	1.36μs	0.535ns	2.07ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.15μs	0.395ns	1.48ns	0.0119	880 B
#3169	`StartFinishSpan`	net472	1.18μs	3.92ns	15.2ns	0.129	810 B
#3169	`StartFinishSpan`	netcoreapp3.1	938ns	0.358ns	1.34ns	0.0103	760 B
#3169	`StartFinishScope`	net472	1.45μs	0.956ns	3.45ns	0.141	891 B
#3169	`StartFinishScope`	netcoreapp3.1	1.04μs	0.899ns	3.36ns	0.0116	880 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.49μs	0.48ns	1.8ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.2μs	2.28ns	8.52ns	0.012	880 B
#3169	`RunOnMethodBegin`	net472	1.43μs	0.639ns	2.39ns	0.141	891 B
#3169	`RunOnMethodBegin`	netcoreapp3.1	1.25μs	0.302ns	1.13ns	0.012	880 B

andrewlock · 2022-09-08T14:50:31Z

Code Coverage Report 📊

✔️ Merging #3169 into master will will increase line coverage by 1%
✔️ Merging #3169 into master will will increase branch coverage by 1%
✔️ Merging #3169 into master will will decrease complexity by 6

	master	#3169	Change
Lines	`17675` / `24344`	`17840` / `24344`
Lines %	`73%`	`73%`	`1%` ✔️
Branches	`10392` / `14964`	`10508` / `14960`
Branches %	`69%`	`70%`	`1%` ✔️
Complexity	`16166`	`16160`	`-6` ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3169	Change
Lines %	`73%`	`73%`	`1%` ✔️
Branches %	`69%`	`70%`	`1%` ✔️
Complexity	`16166`	`16160`	`-6` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Logging.DatadogSerilogLogger	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Debugger.PInvoke.DebuggerNativeMethods	`6%` ✔️	`10%` ✔️	`0` ✔️
Datadog.Trace.Agent.StatsAggregator	`11%` ✔️	`28%` ✔️	`0` ✔️
Datadog.Trace.Agent.Transports.HttpStreamRequestFactory	`12%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.Processors.ObfuscatorTraceProcessor	`12%` ✔️	`15%` ✔️	`0` ✔️
Datadog.Trace.Processors.NormalizerTraceProcessor	`22%` ✔️	`19%` ✔️	`0` ✔️
Datadog.Trace.Telemetry.Transports.JsonTelemetryTransport	`39%` ✔️	`58%` ✔️	`0` ✔️
Datadog.Trace.Util.SamplingHelpers	`50%` ✔️	`50%` ✔️	`0` ✔️
Datadog.Trace.Agent.TraceSamplers.RareSampler	`89%` ✔️	`96%` ✔️	`0` ✔️
...And 4 more

1 classes were removed from Datadog.Trace in #3169

View the full reports for further details:

shurivich requested review from a team as code owners September 5, 2022 12:20

github-actions Bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Sep 5, 2022

shurivich marked this pull request as draft September 5, 2022 12:22

shurivich self-assigned this Sep 5, 2022

shurivich added area:debugger and removed area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) labels Sep 5, 2022

andrewlock approved these changes Sep 5, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/Debugger/Snapshots/DebuggerSnapshotCreator.cs

This comment has been minimized.

Sign in to view

github-actions Bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Sep 6, 2022

shurivich marked this pull request as ready for review September 6, 2022 08:56

shurivich force-pushed the shurivich/remove_redundant_string_allocations_2 branch from dbc7c60 to e174cac Compare September 6, 2022 08:57

This comment has been minimized.

Sign in to view

zacharycmontoya reviewed Sep 6, 2022

View reviewed changes

Comment thread tracer/src/Datadog.Trace/RemoteConfigurationManagement/Transport/RemoteConfigurationFileApi.cs Outdated

OmerRaviv reviewed Sep 7, 2022

View reviewed changes

OmerRaviv approved these changes Sep 7, 2022

View reviewed changes

andrewlock mentioned this pull request Sep 7, 2022

Implement rcte2 - Configuration apply status #3175

Merged

zacharycmontoya approved these changes Sep 7, 2022

View reviewed changes

shurivich force-pushed the shurivich/remove_redundant_string_allocations_2 branch from 8628e08 to 90affbc Compare September 8, 2022 09:29

This comment has been minimized.

Sign in to view

shurivich added 6 commits September 8, 2022 15:43

remove redundant string allocations

1804968

fix _jsonUnderlyingString

e104586

fix DebuggerSnapshotCreator

229f8cb

fix build + use CryptoStream

91d67ab

Revert "fix build + use CryptoStream"

25059b6

This reverts commit 90affbc.

use serializer

10dc0d6

shurivich force-pushed the shurivich/remove_redundant_string_allocations_2 branch from 6004bc4 to 10dc0d6 Compare September 8, 2022 12:45

shurivich merged commit a52eb41 into master Sep 8, 2022

shurivich deleted the shurivich/remove_redundant_string_allocations_2 branch September 8, 2022 14:45

github-actions Bot added this to the vNext milestone Sep 8, 2022

Uh oh!

Conversation

shurivich commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

OmerRaviv Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

shurivich Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shurivich Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

OmerRaviv left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

andrewlock commented Sep 8, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Slower ⚠️ in #3169

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #3169

Raw results

Raw results

Uh oh!

andrewlock commented Sep 8, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shurivich commented Sep 5, 2022 •

edited

Loading

shurivich Sep 8, 2022 •

edited

Loading