Skip to content

[Profiler] Return string view for symbol resolution#3127

Merged
gleocadie merged 5 commits into
masterfrom
gleocadie/return-string_view-for-symbol-resolution-act-2
Sep 1, 2022
Merged

[Profiler] Return string view for symbol resolution#3127
gleocadie merged 5 commits into
masterfrom
gleocadie/return-string_view-for-symbol-resolution-act-2

Conversation

@gleocadie

@gleocadie gleocadie commented Aug 25, 2022

Copy link
Copy Markdown
Collaborator

Summary of changes

Reason for change

When resolving symbols, we return std::string, and those string are copied multiple times, until added to the profile..
By using std::string_view wherever we can we avoid string copies and memory/cpu overhead.

Implementation details

Use std::string_view when resolving symbols.

Test coverage

Ensure that current test are working correctly.

Other details

@gleocadie
gleocadie requested a review from a team as a code owner August 25, 2022 15:19
@gleocadie gleocadie changed the title [Profiler] Return string view for symbol resolution act 2 [Profiler] Return string view for symbol resolution Aug 25, 2022
@github-actions github-actions Bot added the area:profiler Issues related to the continous-profiler label Aug 25, 2022
@gleocadie
gleocadie force-pushed the gleocadie/return-string_view-for-symbol-resolution-act-2 branch from ace9582 to d3a23a5 Compare August 25, 2022 17:35
chrisnas
chrisnas reviewed Aug 26, 2022
View reviewed changes
Comment thread profiler/test/Datadog.Profiler.Native.Tests/LibddprofExporterTest.cpp Outdated
auto exporter = LibddprofExporter(&mockConfiguration, &applicationStore, runtimeInfo, &enabledProfilers);

// Add samples to only one application
auto callstack1 = std::initializer_list<std::pair<std::string, std::string>>({{"module", "frame1"}, {"module", "frame2"}, {"module", "frame3"}});

@chrisnas chrisnas Aug 26, 2022

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.

Good idea to have variables for the frames
Maybe having them as a class field could be the next step: it would avoid repeating them in tests

Comment thread profiler/src/ProfilerEngine/Datadog.Profiler.Native/Sample.h Outdated
Comment thread profiler/src/ProfilerEngine/Datadog.Profiler.Native/Sample.h Outdated
@gleocadie
gleocadie requested review from a team as code owners August 26, 2022 09:25
@andrewlock

This comment has been minimized.

Sign in to view
@gleocadie
gleocadie force-pushed the gleocadie/return-string_view-for-symbol-resolution-act-2 branch from 5f59d20 to 443db37 Compare August 26, 2022 09:58
@andrewlock

andrewlock commented Aug 26, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3127 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 717μs 310ns 1.16μs 0.359 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 468μs 755ns 2.83μs 0 0 0 2.58 KB
#3127 WriteAndFlushEnrichedTraces net472 712μs 457ns 1.77μs 0.353 0 0 3.18 KB
#3127 WriteAndFlushEnrichedTraces netcoreapp3.1 462μs 170ns 659ns 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.129ns 0.5ns 0.0675 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 238ns 0.323ns 1.21ns 0.00584 0 0 424 B
master AllCycleMoreComplexBody net472 181ns 0.159ns 0.616ns 0.0638 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 235ns 0.218ns 0.815ns 0.00542 0 0 400 B
master BodyExtractorSimpleBody net472 273ns 0.221ns 0.827ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 227ns 0.322ns 1.21ns 0.00365 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.6μs 8.46ns 30.5ns 1.21 0.0148 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.6μs 9.94ns 37.2ns 0.0879 0 0 6.75 KB
#3127 AllCycleSimpleBody net472 183ns 0.142ns 0.552ns 0.0676 0 0 425 B
#3127 AllCycleSimpleBody netcoreapp3.1 238ns 0.154ns 0.596ns 0.00573 0 0 424 B
#3127 AllCycleMoreComplexBody net472 181ns 0.193ns 0.747ns 0.0637 0 0 401 B
#3127 AllCycleMoreComplexBody netcoreapp3.1 235ns 0.246ns 0.92ns 0.00555 0 0 400 B
#3127 BodyExtractorSimpleBody net472 259ns 0.232ns 0.898ns 0.0574 0 0 361 B
#3127 BodyExtractorSimpleBody netcoreapp3.1 234ns 0.31ns 1.2ns 0.00363 0 0 272 B
#3127 BodyExtractorMoreComplexBody net472 14.8μs 12.7ns 47.7ns 1.21 0.0149 0 7.62 KB
#3127 BodyExtractorMoreComplexBody netcoreapp3.1 12μs 13.3ns 51.6ns 0.0901 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 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 180μs 170ns 659ns 0.268 0 0 20.57 KB
#3127 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3127 SendRequest netcoreapp3.1 182μs 162ns 627ns 0.273 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.78μs 1.18ns 4.4ns 0.15 0.000887 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.45μs 0.516ns 2ns 0.0124 0 0 936 B
#3127 ExecuteNonQuery net472 1.76μs 0.643ns 2.49ns 0.15 0.00087 0 947 B
#3127 ExecuteNonQuery netcoreapp3.1 1.35μs 0.381ns 1.43ns 0.0121 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.52μs 0.693ns 2.68ns 0.184 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.53μs 1.33ns 5.16ns 0.015 0 0 1.1 KB
master CallElasticsearchAsync net472 2.61μs 1.4ns 5.24ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.6μs 1.23ns 4.59ns 0.016 0 0 1.22 KB
#3127 CallElasticsearch net472 2.51μs 0.938ns 3.63ns 0.183 0 0 1.16 KB
#3127 CallElasticsearch netcoreapp3.1 1.49μs 1.52ns 5.67ns 0.0148 0 0 1.1 KB
#3127 CallElasticsearchAsync net472 2.5μs 1.1ns 4.25ns 0.205 0 0 1.29 KB
#3127 CallElasticsearchAsync netcoreapp3.1 1.58μs 1.62ns 6.05ns 0.0166 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.67μs 5.7ns 22.1ns 0.224 0 0 1.41 KB
master ExecuteAsync netcoreapp3.1 1.66μs 2.01ns 7.53ns 0.0186 0 0 1.34 KB
#3127 ExecuteAsync net472 2.53μs 11.1ns 40.2ns 0.224 0 0 1.41 KB
#3127 ExecuteAsync netcoreapp3.1 1.62μs 4.83ns 18.7ns 0.0185 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.69μs 11ns 41.1ns 0.438 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.5μs 5.49ns 20.5ns 0.0353 0 0 2.6 KB
#3127 SendAsync net472 5.73μs 14.1ns 54.5ns 0.439 0 0 2.77 KB
#3127 SendAsync netcoreapp3.1 3.6μs 7.6ns 29.4ns 0.034 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.18μs 1.38ns 5.16ns 0.287 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.57μs 1.17ns 4.2ns 0.0244 0 0 1.85 KB
#3127 EnrichedLog net472 2.99μs 0.928ns 3.35ns 0.288 0 0 1.81 KB
#3127 EnrichedLog netcoreapp3.1 2.54μs 1.02ns 3.8ns 0.0252 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 148μs 74.5ns 279ns 0.672 0.224 0 4.65 KB
master EnrichedLog netcoreapp3.1 115μs 167ns 646ns 0 0 0 4.49 KB
#3127 EnrichedLog net472 148μs 229ns 888ns 0.676 0.225 0 4.65 KB
#3127 EnrichedLog netcoreapp3.1 116μs 164ns 613ns 0 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.5μs 13.2ns 49.3ns 0.568 0.00272 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.29μs 12.3ns 47.7ns 0.054 0 0 3.91 KB
#3127 EnrichedLog net472 5.58μs 9.47ns 36.7ns 0.568 0.00285 0 3.59 KB
#3127 EnrichedLog netcoreapp3.1 4.29μs 13.5ns 52.2ns 0.0528 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.19μs 1.31ns 5.09ns 0.218 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.8μs 1.18ns 4.42ns 0.0181 0 0 1.32 KB
#3127 SendReceive net472 2.15μs 0.395ns 1.53ns 0.217 0 0 1.37 KB
#3127 SendReceive netcoreapp3.1 1.81μs 0.977ns 3.78ns 0.0182 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.01μs 1.91ns 7.13ns 0.354 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.25μs 3.53ns 13.7ns 0.0234 0 0 1.8 KB
#3127 EnrichedLog net472 5.06μs 2.15ns 8.04ns 0.354 0 0 2.23 KB
#3127 EnrichedLog netcoreapp3.1 4.31μs 1.1ns 4.27ns 0.0236 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.901ns 3.37ns 0.128 0 0 810 B
master StartFinishSpan netcoreapp3.1 895ns 0.448ns 1.74ns 0.0102 0 0 760 B
master StartFinishScope net472 1.38μs 0.87ns 3.37ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.05μs 0.909ns 3.52ns 0.012 0 0 880 B
#3127 StartFinishSpan net472 1.11μs 1.12ns 4.34ns 0.128 0 0 810 B
#3127 StartFinishSpan netcoreapp3.1 874ns 1.05ns 3.93ns 0.0105 0 0 760 B
#3127 StartFinishScope net472 1.37μs 1.23ns 4.78ns 0.141 0 0 891 B
#3127 StartFinishScope netcoreapp3.1 1.07μs 1.45ns 5.24ns 0.0115 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.52μs 0.914ns 3.54ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.15μs 0.502ns 1.88ns 0.0121 0 0 880 B
#3127 RunOnMethodBegin net472 1.44μs 2.42ns 9.06ns 0.141 0 0 891 B
#3127 RunOnMethodBegin netcoreapp3.1 1.14μs 1.17ns 4.55ns 0.012 0 0 880 B

@gleocadie
gleocadie force-pushed the gleocadie/return-string_view-for-symbol-resolution-act-2 branch 14 times, most recently from 2971862 to 80030bf Compare August 31, 2022 20:54
@gleocadie
gleocadie force-pushed the gleocadie/return-string_view-for-symbol-resolution-act-2 branch from 80030bf to 0a0a00b Compare August 31, 2022 21:01
chrisnas
chrisnas approved these changes Sep 1, 2022
View reviewed changes

@chrisnas chrisnas 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.

LGTM

@gleocadie
gleocadie merged commit 855afa2 into master Sep 1, 2022
@gleocadie
gleocadie deleted the gleocadie/return-string_view-for-symbol-resolution-act-2 branch September 1, 2022 12:55
@github-actions github-actions Bot added this to the vNext milestone Sep 1, 2022
kevingosse pushed a commit that referenced this pull request Sep 1, 2022
@gleocadie @chrisnas
[Profiler] Return string view for symbol resolution (#3127)
07dc603 
* Run unit tests with ASAN
* Run unit tests with UBSAN

Co-authored-by: chrisnas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrisnas chrisnas chrisnas approved these changes

Assignees

No one assigned

Labels

area:profiler Issues related to the continous-profiler

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@gleocadie @andrewlock @chrisnas