Skip to content

Remove redundant string allocations for ApiResponses#3144

Merged
shurivich merged 4 commits into
masterfrom
shurivich/remove_redundant_string_allocations
Sep 4, 2022
Merged

Remove redundant string allocations for ApiResponses#3144
shurivich merged 4 commits into
masterfrom
shurivich/remove_redundant_string_allocations

Conversation

@shurivich

@shurivich shurivich commented Aug 31, 2022

Copy link
Copy Markdown
Contributor

Summary of changes

Introduce GetStreamAsync method for IApiResponse.
Introduce ReadAsStringAsync and ReadAsTypeAsync extension methods.

Reason for change

Right now we are allocating IApiResponse content to strings and afterwards converting them to object.
This PR removes redundant string allocations, by using JsonSerializer ability to read streams.

@shurivich
shurivich requested review from a team as code owners August 31, 2022 13:18
@shurivich shurivich self-assigned this Aug 31, 2022
@andrewlock

This comment has been minimized.

Sign in to view
andrewlock
andrewlock approved these changes Aug 31, 2022
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

zacharycmontoya
zacharycmontoya approved these changes Aug 31, 2022
View reviewed changes

@zacharycmontoya zacharycmontoya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

This comment has been minimized.

Sign in to view
OmerRaviv
OmerRaviv approved these changes Sep 4, 2022
View reviewed changes

@OmerRaviv OmerRaviv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very cool improvement, thanks!

@andrewlock

This comment has been minimized.

Sign in to view
@shurivich
shurivich force-pushed the shurivich/remove_redundant_string_allocations branch from 068f866 to 55197f7 Compare September 4, 2022 12:56
@andrewlock

andrewlock commented Sep 4, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3144 compared to master:

  • All benchmarks have the same speed
  • 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 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 720μs 517ns 2μs 0.359 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 471μs 265ns 992ns 0 0 0 2.58 KB
#3144 WriteAndFlushEnrichedTraces net472 715μs 294ns 1.06μs 0.357 0 0 3.18 KB
#3144 WriteAndFlushEnrichedTraces netcoreapp3.1 483μs 277ns 1.07μs 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 185ns 0.17ns 0.637ns 0.0676 9.26E-05 0 425 B
master AllCycleSimpleBody netcoreapp3.1 236ns 0.546ns 2.12ns 0.00577 0 0 424 B
master AllCycleMoreComplexBody net472 184ns 0.0994ns 0.358ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 248ns 0.198ns 0.766ns 0.00547 0 0 400 B
master BodyExtractorSimpleBody net472 266ns 0.261ns 0.975ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 222ns 0.12ns 0.416ns 0.00369 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.3μs 7.15ns 25.8ns 1.21 0.0213 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12μs 13.9ns 50.1ns 0.0895 0 0 6.75 KB
#3144 AllCycleSimpleBody net472 187ns 0.286ns 1.07ns 0.0675 0 0 425 B
#3144 AllCycleSimpleBody netcoreapp3.1 238ns 0.365ns 1.36ns 0.00587 0 0 424 B
#3144 AllCycleMoreComplexBody net472 184ns 0.13ns 0.467ns 0.0637 0 0 401 B
#3144 AllCycleMoreComplexBody netcoreapp3.1 233ns 0.197ns 0.736ns 0.00539 0 0 400 B
#3144 BodyExtractorSimpleBody net472 254ns 0.25ns 0.935ns 0.0573 0 0 361 B
#3144 BodyExtractorSimpleBody netcoreapp3.1 224ns 0.3ns 1.16ns 0.00362 0 0 272 B
#3144 BodyExtractorMoreComplexBody net472 14.1μs 8ns 28.8ns 1.2 0.0214 0 7.62 KB
#3144 BodyExtractorMoreComplexBody netcoreapp3.1 12.1μs 14.4ns 55.7ns 0.0912 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 183μs 272ns 1.02μs 0.273 0 0 20.57 KB
#3144 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3144 SendRequest netcoreapp3.1 183μs 178ns 689ns 0.183 0 0 20.57 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net472 1.89μs 0.459ns 1.78ns 0.15 0.000948 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.53μs 0.982ns 3.8ns 0.0122 0 0 936 B
#3144 ExecuteNonQuery net472 1.85μs 0.552ns 2.07ns 0.15 0.000917 0 947 B
#3144 ExecuteNonQuery netcoreapp3.1 1.45μs 0.476ns 1.65ns 0.0123 0 0 936 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net472 2.64μs 0.925ns 3.58ns 0.184 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.53μs 2.07ns 8.03ns 0.0143 0 0 1.1 KB
master CallElasticsearchAsync net472 2.61μs 0.405ns 1.52ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.63μs 0.533ns 2ns 0.0164 0 0 1.22 KB
#3144 CallElasticsearch net472 2.51μs 0.945ns 3.27ns 0.183 0 0 1.16 KB
#3144 CallElasticsearch netcoreapp3.1 1.52μs 0.98ns 3.8ns 0.0144 0 0 1.1 KB
#3144 CallElasticsearchAsync net472 2.65μs 0.82ns 2.96ns 0.204 0 0 1.29 KB
#3144 CallElasticsearchAsync netcoreapp3.1 1.63μs 0.629ns 2.35ns 0.0163 0 0 1.22 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net472 2.62μs 4.02ns 15ns 0.225 0 0 1.42 KB
master ExecuteAsync netcoreapp3.1 1.67μs 2.15ns 8.31ns 0.0176 0 0 1.34 KB
#3144 ExecuteAsync net472 2.7μs 7.07ns 27.4ns 0.225 0 0 1.42 KB
#3144 ExecuteAsync netcoreapp3.1 1.67μs 4.81ns 18.6ns 0.0186 0 0 1.34 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 5.66μs 8.6ns 31ns 0.437 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.68μs 9.98ns 38.7ns 0.0342 0 0 2.6 KB
#3144 SendAsync net472 5.8μs 14.9ns 57.6ns 0.438 0 0 2.77 KB
#3144 SendAsync netcoreapp3.1 3.58μs 7.11ns 27.5ns 0.0361 0 0 2.6 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 3.14μs 2.16ns 8.38ns 0.288 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.59μs 1.55ns 5.79ns 0.0245 0 0 1.85 KB
#3144 EnrichedLog net472 3.17μs 1.29ns 4.83ns 0.287 0 0 1.81 KB
#3144 EnrichedLog netcoreapp3.1 2.47μs 0.711ns 2.66ns 0.0246 0 0 1.85 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 150μs 157ns 609ns 0.672 0.224 0 4.65 KB
master EnrichedLog netcoreapp3.1 117μs 105ns 407ns 0.058 0 0 4.49 KB
#3144 EnrichedLog net472 151μs 163ns 630ns 0.683 0.228 0 4.65 KB
#3144 EnrichedLog netcoreapp3.1 115μs 42.4ns 159ns 0.0573 0 0 4.49 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.71μs 13.8ns 53.6ns 0.57 0.00288 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.37μs 10ns 38.8ns 0.0531 0 0 3.91 KB
#3144 EnrichedLog net472 5.64μs 14ns 54.4ns 0.57 0.00275 0 3.59 KB
#3144 EnrichedLog netcoreapp3.1 4.59μs 8.99ns 34.8ns 0.0541 0 0 3.91 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net472 2.34μs 2.85ns 11.1ns 0.217 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.89μs 0.466ns 1.74ns 0.0181 0 0 1.32 KB
#3144 SendReceive net472 2.32μs 1.82ns 7.07ns 0.218 0 0 1.37 KB
#3144 SendReceive netcoreapp3.1 1.83μs 0.517ns 1.86ns 0.0175 0 0 1.32 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net472 5.03μs 2.59ns 10ns 0.353 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.5μs 0.927ns 3.47ns 0.0225 0 0 1.8 KB
#3144 EnrichedLog net472 5.03μs 1.24ns 4.63ns 0.353 0 0 2.23 KB
#3144 EnrichedLog netcoreapp3.1 4.39μs 3.4ns 12.7ns 0.0243 0 0 1.8 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 1.12μs 0.521ns 1.95ns 0.128 0 0 810 B
master StartFinishSpan netcoreapp3.1 929ns 0.708ns 2.65ns 0.0102 0 0 760 B
master StartFinishScope net472 1.44μs 1.45ns 5.44ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.14μs 0.499ns 1.87ns 0.0119 0 0 880 B
#3144 StartFinishSpan net472 1.18μs 0.911ns 3.29ns 0.128 0 0 810 B
#3144 StartFinishSpan netcoreapp3.1 1μs 0.656ns 2.54ns 0.00998 0 0 760 B
#3144 StartFinishScope net472 1.4μs 0.275ns 0.993ns 0.141 0 0 891 B
#3144 StartFinishScope netcoreapp3.1 1.06μs 0.381ns 1.42ns 0.0117 0 0 880 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net472 1.49μs 0.919ns 3.56ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.16μs 0.549ns 1.98ns 0.0121 0 0 880 B
#3144 RunOnMethodBegin net472 1.46μs 0.403ns 1.51ns 0.141 0 0 891 B
#3144 RunOnMethodBegin netcoreapp3.1 1.15μs 0.284ns 1.06ns 0.012 0 0 880 B

@andrewlock

andrewlock commented Sep 4, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

✔️ Merging #3144 into master will not change line coverage
✔️ Merging #3144 into master will not change branch coverage
⛔ Merging #3144 into master will will increase complexity by 12

master #3144 Change
Lines 17566 / 24155 17562 / 24154
Lines % 73% 73% 0% ✔️
Branches 10400 / 14873 10398 / 14883
Branches % 70% 70% 0% ✔️
Complexity 16056 16068 12 ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3144 Change
Lines % 73% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 16056 16068 12 ⛔

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Agent.Transports.SocketHandlerRequestFactory -7% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.Sink.AgentBatchUploadApi -1% ⚠️ -17% ⛔ -2 ✔️
Datadog.Trace.Agent.Transports.HttpClientResponse 4% ✔️ 12% ✔️ 7 ⛔
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️
Datadog.Trace.Agent.DiscoveryService.DiscoveryService 10% ✔️ 0% ✔️ 0 ✔️

The following classes were added in #3144:

File Line coverage Branch coverage Complexity
Datadog.Trace.Agent.ApiResponseExtensions 100% 100% 3

View the full reports for further details:

@shurivich
shurivich merged commit 767f231 into master Sep 4, 2022
@shurivich
shurivich deleted the shurivich/remove_redundant_string_allocations branch September 4, 2022 14:14
@github-actions github-actions Bot added this to the vNext milestone Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OmerRaviv OmerRaviv OmerRaviv approved these changes
@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes
@andrewlock andrewlock andrewlock approved these changes

Assignees

@shurivich shurivich

Labels

None yet

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

4 participants

@shurivich @andrewlock @OmerRaviv @zacharycmontoya