Skip to content

[ASM] Improved Unsafe Encoder readability #5587

Merged
dromanol merged 5 commits into
masterfrom
dani/asm/encoder_readability_refactor
Jun 6, 2024
Merged

[ASM] Improved Unsafe Encoder readability #5587
dromanol merged 5 commits into
masterfrom
dani/asm/encoder_readability_refactor

Conversation

@dromanol

@dromanol dromanol commented May 20, 2024

Copy link
Copy Markdown
Contributor

Summary of changes

Externalized all embedded functions in Encoder

Reason for change

Encoder code was a bit difficult to read due to the long embedded functions it contained.

Implementation details

Created a EncoderContext struct passed by ref to all calls.
Externalized all embedded functions passing previously captured data as arguments

@dromanol
dromanol requested a review from a team as a code owner May 20, 2024 16:34
@dromanol dromanol mentioned this pull request May 20, 2024
Closed
@datadog-ddstaging

datadog-ddstaging Bot commented May 20, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: dani/asm/encoder_readability_refactor
Commit report: adf044f
Test service: dd-trace-dotnet

✅ 0 Failed, 340912 Passed, 1572 Skipped, 14h 26m 8.28s Total Time

@andrewlock

andrewlock commented May 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 (5587) - mean (71ms)  : 64, 78
     .   : milestone, 71,
    master - mean (73ms)  : 65, 81
     .   : milestone, 73,

    section CallTarget+Inlining+NGEN
    This PR (5587) - mean (996ms)  : 974, 1019
     .   : milestone, 996,
    master - mean (990ms)  : 973, 1008
     .   : milestone, 990,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5587) - mean (109ms)  : 106, 112
     .   : milestone, 109,
    master - mean (109ms)  : 106, 112
     .   : milestone, 109,

    section CallTarget+Inlining+NGEN
    This PR (5587) - mean (714ms)  : 692, 735
     .   : milestone, 714,
    master - mean (710ms)  : 690, 730
     .   : milestone, 710,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5587) - mean (92ms)  : 90, 95
     .   : milestone, 92,
    master - mean (93ms)  : 90, 96
     .   : milestone, 93,

    section CallTarget+Inlining+NGEN
    This PR (5587) - mean (670ms)  : 652, 688
     .   : milestone, 670,
    master - mean (664ms)  : 642, 687
     .   : milestone, 664,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5587) - mean (195ms)  : 191, 199
     .   : milestone, 195,
    master - mean (194ms)  : 189, 199
     .   : milestone, 194,

    section CallTarget+Inlining+NGEN
    This PR (5587) - mean (1,096ms)  : 1078, 1115
     .   : milestone, 1096,
    master - mean (1,094ms)  : 1066, 1123
     .   : milestone, 1094,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5587) - mean (278ms)  : 272, 283
     .   : milestone, 278,
    master - mean (278ms)  : 272, 284
     .   : milestone, 278,

    section CallTarget+Inlining+NGEN
    This PR (5587) - mean (902ms)  : 877, 926
     .   : milestone, 902,
    master - mean (892ms)  : 873, 911
     .   : milestone, 892,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5587) - mean (270ms)  : 263, 276
     .   : milestone, 270,
    master - mean (268ms)  : 262, 274
     .   : milestone, 268,

    section CallTarget+Inlining+NGEN
    This PR (5587) - mean (874ms)  : 856, 893
     .   : milestone, 874,
    master - mean (880ms)  : 851, 909
     .   : milestone, 880,
Loading

@andrewlock

andrewlock commented May 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 (5587) (11.816M)   : 0, 11815884
    master (11.636M)   : 0, 11636380
    benchmarks/2.9.0 (11.377M)   : 0, 11377344

    section Automatic
    This PR (5587) (7.849M)   : 0, 7848927
    master (8.032M)   : 0, 8032165
    benchmarks/2.9.0 (7.933M)   : 0, 7933446

    section Trace stats
    master (8.282M)   : 0, 8281771

    section Manual
    This PR (5587) (9.867M)   : 0, 9867092
    master (10.260M)   : 0, 10259702

    section Manual + Automatic
    This PR (5587) (7.064M)   : crit ,0, 7064081
    master (7.604M)   : 0, 7603906

    section Version Conflict
    master (6.818M)   : 0, 6817845
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5587) (9.629M)   : 0, 9628907
    master (9.595M)   : 0, 9595439
    benchmarks/2.9.0 (9.678M)   : 0, 9678191

    section Automatic
    This PR (5587) (6.717M)   : 0, 6716910
    master (6.565M)   : 0, 6564640

    section Trace stats
    master (6.911M)   : 0, 6911499

    section Manual
    This PR (5587) (8.340M)   : 0, 8339841
    master (8.321M)   : 0, 8320871

    section Manual + Automatic
    This PR (5587) (6.218M)   : 0, 6218299
    master (6.057M)   : 0, 6056637

    section Version Conflict
    master (5.545M)   : 0, 5545291
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5587) (9.666M)   : 0, 9665754
    master (10.152M)   : 0, 10151846
    benchmarks/2.9.0 (9.921M)   : 0, 9920753

    section Automatic
    This PR (5587) (6.815M)   : 0, 6814854
    master (6.997M)   : 0, 6996799
    benchmarks/2.9.0 (7.177M)   : 0, 7176540

    section Trace stats
    master (7.347M)   : 0, 7346895

    section Manual
    This PR (5587) (8.527M)   : 0, 8526522
    master (8.845M)   : 0, 8845027

    section Manual + Automatic
    This PR (5587) (6.699M)   : 0, 6698583
    master (6.836M)   : 0, 6835982

    section Version Conflict
    master (6.125M)   : 0, 6124943
Loading

@andrewlock

andrewlock commented May 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for tracer 🐌

Benchmarks for #5587 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.123
  • 2 benchmarks are slower, with geometric mean 1.143
  • 1 benchmarks have more 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.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 8.77μs 46ns 234ns 0.0173 0.00433 0 7.56 KB
master StartStopWithChild netcoreapp3.1 10.8μs 56.4ns 382ns 0.0267 0.0107 0 7.65 KB
master StartStopWithChild net472 17.7μs 94.1ns 470ns 1.36 0.385 0.0985 8.06 KB
#5587 StartStopWithChild net6.0 8.82μs 46.9ns 248ns 0.0174 0.00435 0 7.56 KB
#5587 StartStopWithChild netcoreapp3.1 10.8μs 57ns 296ns 0.0291 0.0117 0 7.65 KB
#5587 StartStopWithChild net472 18.5μs 64.8ns 242ns 1.33 0.331 0.0896 8.05 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 460μs 189ns 707ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 618μs 212ns 820ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 853μs 314ns 1.17μs 0.428 0 0 3.3 KB
#5587 WriteAndFlushEnrichedTraces net6.0 493μs 83.1ns 311ns 0 0 0 2.7 KB
#5587 WriteAndFlushEnrichedTraces netcoreapp3.1 634μs 157ns 588ns 0 0 0 2.7 KB
#5587 WriteAndFlushEnrichedTraces net472 846μs 241ns 933ns 0.422 0 0 3.3 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 net6.0 171μs 276ns 1.07μs 0.253 0 0 18.44 KB
master SendRequest netcoreapp3.1 195μs 371ns 1.44μs 0.196 0 0 20.6 KB
master SendRequest net472 0.000614ns 0.000246ns 0.000922ns 0 0 0 0 b
#5587 SendRequest net6.0 173μs 192ns 719ns 0.256 0 0 18.44 KB
#5587 SendRequest netcoreapp3.1 192μs 408ns 1.58μs 0.194 0 0 20.6 KB
#5587 SendRequest net472 0.000626ns 0.000236ns 0.000849ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5587

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑net6.0 41.38 KB 41.69 KB 304 B 0.73%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 537μs 1.31μs 5.08μs 0.541 0 0 41.38 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 644μs 710ns 2.56μs 0.319 0 0 41.68 KB
master WriteAndFlushEnrichedTraces net472 858μs 3.84μs 14.9μs 8.3 2.62 0.437 53.24 KB
#5587 WriteAndFlushEnrichedTraces net6.0 551μs 360ns 1.4μs 0.546 0 0 41.69 KB
#5587 WriteAndFlushEnrichedTraces netcoreapp3.1 659μs 1.74μs 6.72μs 0.324 0 0 41.86 KB
#5587 WriteAndFlushEnrichedTraces net472 856μs 3.37μs 13.1μs 8.33 2.5 0.417 53.25 KB
Benchmarks.Trace.DbCommandBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5587

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net6.0 1.168 1,072.86 1,253.43

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.07μs 0.479ns 1.85ns 0.0112 0 0 808 B
master ExecuteNonQuery netcoreapp3.1 1.53μs 1.05ns 4.06ns 0.0108 0 0 808 B
master ExecuteNonQuery net472 1.7μs 1.83ns 7.1ns 0.122 0 0 770 B
#5587 ExecuteNonQuery net6.0 1.25μs 0.823ns 3.19ns 0.0112 0 0 808 B
#5587 ExecuteNonQuery netcoreapp3.1 1.52μs 0.949ns 3.67ns 0.0106 0 0 808 B
#5587 ExecuteNonQuery net472 1.73μs 1.5ns 5.8ns 0.122 0 0 770 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 net6.0 1.23μs 0.953ns 3.56ns 0.0135 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.5μs 0.752ns 2.91ns 0.013 0 0 976 B
master CallElasticsearch net472 2.61μs 1.95ns 7.53ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.3μs 0.501ns 1.87ns 0.013 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.68μs 1.83ns 7.08ns 0.0142 0 0 1.02 KB
master CallElasticsearchAsync net472 2.66μs 1.26ns 4.87ns 0.167 0 0 1.05 KB
#5587 CallElasticsearch net6.0 1.23μs 0.597ns 2.31ns 0.0135 0 0 976 B
#5587 CallElasticsearch netcoreapp3.1 1.56μs 1.37ns 5.12ns 0.0132 0 0 976 B
#5587 CallElasticsearch net472 2.51μs 1.02ns 3.95ns 0.158 0.00125 0 995 B
#5587 CallElasticsearchAsync net6.0 1.37μs 1.28ns 4.95ns 0.013 0 0 952 B
#5587 CallElasticsearchAsync netcoreapp3.1 1.63μs 0.712ns 2.76ns 0.0139 0 0 1.02 KB
#5587 CallElasticsearchAsync net472 2.59μs 2.01ns 7.79ns 0.167 0 0 1.05 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 net6.0 1.27μs 0.576ns 2.15ns 0.0134 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.62μs 1.06ns 3.98ns 0.0129 0 0 952 B
master ExecuteAsync net472 1.81μs 0.495ns 1.85ns 0.145 0 0 915 B
#5587 ExecuteAsync net6.0 1.24μs 0.405ns 1.46ns 0.0131 0 0 952 B
#5587 ExecuteAsync netcoreapp3.1 1.65μs 1.03ns 3.87ns 0.0124 0 0 952 B
#5587 ExecuteAsync net472 1.75μs 0.614ns 2.38ns 0.145 0 0 915 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.21μs 0.998ns 3.6ns 0.0316 0 0 2.22 KB
master SendAsync netcoreapp3.1 5.04μs 1.72ns 6.65ns 0.0381 0 0 2.76 KB
master SendAsync net472 7.48μs 4.54ns 17.6ns 0.494 0 0 3.12 KB
#5587 SendAsync net6.0 4.03μs 1.04ns 3.89ns 0.0302 0 0 2.22 KB
#5587 SendAsync netcoreapp3.1 5.08μs 2.68ns 10.4ns 0.0356 0 0 2.76 KB
#5587 SendAsync net472 7.63μs 2.13ns 7.97ns 0.495 0 0 3.12 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 net6.0 1.43μs 0.486ns 1.82ns 0.0228 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.27μs 0.741ns 2.77ns 0.0229 0 0 1.64 KB
master EnrichedLog net472 2.6μs 1.76ns 6.57ns 0.249 0 0 1.57 KB
#5587 EnrichedLog net6.0 1.58μs 0.325ns 1.17ns 0.023 0 0 1.64 KB
#5587 EnrichedLog netcoreapp3.1 2.28μs 1.34ns 5.21ns 0.0216 0 0 1.64 KB
#5587 EnrichedLog net472 2.66μs 2.53ns 9.46ns 0.249 0 0 1.57 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 net6.0 118μs 148ns 572ns 0.0584 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 122μs 136ns 509ns 0.0612 0 0 4.28 KB
master EnrichedLog net472 151μs 62.5ns 234ns 0.676 0.225 0 4.46 KB
#5587 EnrichedLog net6.0 118μs 174ns 650ns 0 0 0 4.28 KB
#5587 EnrichedLog netcoreapp3.1 122μs 189ns 732ns 0.0609 0 0 4.28 KB
#5587 EnrichedLog net472 152μs 125ns 485ns 0.681 0.227 0 4.46 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 net6.0 3.09μs 0.782ns 3.03ns 0.0307 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.06μs 1.82ns 7.06ns 0.0284 0 0 2.2 KB
master EnrichedLog net472 4.75μs 1.44ns 5.39ns 0.32 0 0 2.02 KB
#5587 EnrichedLog net6.0 2.97μs 0.908ns 3.52ns 0.0311 0 0 2.2 KB
#5587 EnrichedLog netcoreapp3.1 4.33μs 2.45ns 9.48ns 0.0282 0 0 2.2 KB
#5587 EnrichedLog net472 5.09μs 1.54ns 5.95ns 0.318 0 0 2.02 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 net6.0 1.31μs 0.881ns 3.3ns 0.0164 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.78μs 0.689ns 2.67ns 0.0151 0 0 1.14 KB
master SendReceive net472 2.12μs 1.76ns 6.83ns 0.183 0 0 1.16 KB
#5587 SendReceive net6.0 1.41μs 1.13ns 4.37ns 0.0162 0 0 1.14 KB
#5587 SendReceive netcoreapp3.1 1.74μs 1.89ns 7.06ns 0.0154 0 0 1.14 KB
#5587 SendReceive net472 2.09μs 2ns 7.75ns 0.183 0 0 1.16 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 net6.0 2.76μs 0.551ns 1.91ns 0.0221 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.88μs 1.13ns 4.24ns 0.0216 0 0 1.65 KB
master EnrichedLog net472 4.46μs 2.63ns 10.2ns 0.322 0 0 2.04 KB
#5587 EnrichedLog net6.0 2.94μs 1.16ns 4.36ns 0.0219 0 0 1.6 KB
#5587 EnrichedLog netcoreapp3.1 3.93μs 2.05ns 7.94ns 0.0215 0 0 1.65 KB
#5587 EnrichedLog net472 4.41μs 2.11ns 8.18ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5587

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 1.118 541.75 605.48

Faster 🎉 in #5587

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 1.123 437.76 389.84

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 437ns 0.477ns 1.85ns 0.00813 0 0 576 B
master StartFinishSpan netcoreapp3.1 542ns 0.189ns 0.682ns 0.00791 0 0 576 B
master StartFinishSpan net472 656ns 0.819ns 3.17ns 0.0916 0 0 578 B
master StartFinishScope net6.0 474ns 0.286ns 1.11ns 0.0098 0 0 696 B
master StartFinishScope netcoreapp3.1 782ns 0.359ns 1.39ns 0.00945 0 0 696 B
master StartFinishScope net472 858ns 0.898ns 3.36ns 0.104 0 0 658 B
#5587 StartFinishSpan net6.0 390ns 0.203ns 0.784ns 0.00799 0 0 576 B
#5587 StartFinishSpan netcoreapp3.1 605ns 0.352ns 1.27ns 0.00786 0 0 576 B
#5587 StartFinishSpan net472 657ns 0.735ns 2.84ns 0.0916 0 0 578 B
#5587 StartFinishScope net6.0 468ns 0.164ns 0.615ns 0.00967 0 0 696 B
#5587 StartFinishScope netcoreapp3.1 736ns 0.319ns 1.24ns 0.00931 0 0 696 B
#5587 StartFinishScope net472 842ns 1.35ns 5.21ns 0.104 0 0 658 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 net6.0 591ns 0.329ns 1.28ns 0.00977 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 919ns 0.486ns 1.82ns 0.0096 0 0 696 B
master RunOnMethodBegin net472 1.1μs 1.39ns 5.37ns 0.104 0 0 658 B
#5587 RunOnMethodBegin net6.0 656ns 0.367ns 1.37ns 0.00988 0 0 696 B
#5587 RunOnMethodBegin netcoreapp3.1 935ns 1.83ns 7.08ns 0.00938 0 0 696 B
#5587 RunOnMethodBegin net472 1.05μs 1.56ns 6.05ns 0.104 0 0 658 B

@andrewlock

andrewlock commented May 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for appsec 🐌

Benchmarks for #5587 compared to master:

  • 2 benchmarks are faster, with geometric mean 1.165
  • 1 benchmarks are slower, with geometric mean 1.355
  • 2 benchmarks have more 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.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5587

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net6.0 1.355 144.26 195.53

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 74.3μs 97.3ns 364ns 0.074 0 0 6 KB
master AllCycleSimpleBody netcoreapp3.1 64μs 77.7ns 291ns 0.0954 0 0 6.94 KB
master AllCycleSimpleBody net472 50.5μs 80.1ns 300ns 1.31 0 0 8.33 KB
master AllCycleMoreComplexBody net6.0 80μs 62.8ns 243ns 0.12 0 0 9.5 KB
master AllCycleMoreComplexBody netcoreapp3.1 71.3μs 78.1ns 281ns 0.108 0 0 10.36 KB
master AllCycleMoreComplexBody net472 57.8μs 79.4ns 307ns 1.87 0.0288 0 11.84 KB
master ObjectExtractorSimpleBody net6.0 144ns 0.149ns 0.557ns 0.00394 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 212ns 0.134ns 0.502ns 0.00382 0 0 272 B
master ObjectExtractorSimpleBody net472 182ns 0.39ns 1.51ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.04μs 2.09ns 7.81ns 0.0535 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 4.02μs 2.71ns 10.5ns 0.0503 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.98μs 4.05ns 15.7ns 0.602 0.00594 0 3.8 KB
#5587 AllCycleSimpleBody net6.0 76.1μs 149ns 577ns 0.0739 0 0 6 KB
#5587 AllCycleSimpleBody netcoreapp3.1 62.9μs 73.2ns 283ns 0.095 0 0 6.94 KB
#5587 AllCycleSimpleBody net472 50.7μs 119ns 463ns 1.31 0 0 8.33 KB
#5587 AllCycleMoreComplexBody net6.0 82.4μs 331ns 1.28μs 0.121 0 0 9.5 KB
#5587 AllCycleMoreComplexBody netcoreapp3.1 71μs 117ns 452ns 0.14 0 0 10.36 KB
#5587 AllCycleMoreComplexBody net472 58.2μs 80.9ns 313ns 1.87 0.0288 0 11.84 KB
#5587 ObjectExtractorSimpleBody net6.0 196ns 0.147ns 0.569ns 0.00396 0 0 280 B
#5587 ObjectExtractorSimpleBody netcoreapp3.1 233ns 0.883ns 3.42ns 0.00368 0 0 272 B
#5587 ObjectExtractorSimpleBody net472 185ns 0.398ns 1.54ns 0.0446 0 0 281 B
#5587 ObjectExtractorMoreComplexBody net6.0 3.1μs 4.69ns 17.5ns 0.0524 0 0 3.78 KB
#5587 ObjectExtractorMoreComplexBody netcoreapp3.1 4.02μs 2.12ns 8.2ns 0.0505 0 0 3.69 KB
#5587 ObjectExtractorMoreComplexBody net472 4.09μs 2.8ns 10.5ns 0.602 0.00613 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 41.3μs 21.5ns 83.1ns 0.454 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 56.3μs 62.1ns 241ns 0.447 0 0 32.4 KB
master EncodeArgs net472 71.1μs 62.2ns 241ns 5.15 0.0711 0 32.5 KB
master EncodeLegacyArgs net6.0 71.8μs 91.2ns 353ns 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 105μs 119ns 459ns 0 0 0 2.14 KB
master EncodeLegacyArgs net472 155μs 118ns 459ns 0.308 0 0 2.15 KB
#5587 EncodeArgs net6.0 38.3μs 41.8ns 162ns 0.46 0 0 32.4 KB
#5587 EncodeArgs netcoreapp3.1 54μs 29.4ns 114ns 0.457 0 0 32.4 KB
#5587 EncodeArgs net472 68.9μs 48.9ns 176ns 5.13 0.0689 0 32.5 KB
#5587 EncodeLegacyArgs net6.0 71.9μs 29.2ns 113ns 0.0377 0 0 2.14 KB
#5587 EncodeLegacyArgs netcoreapp3.1 106μs 94.8ns 367ns 0 0 0 2.14 KB
#5587 EncodeLegacyArgs net472 157μs 88ns 341ns 0.314 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 186μs 127ns 492ns 0 0 0 2.42 KB
master RunWafRealisticBenchmark netcoreapp3.1 199μs 195ns 731ns 0 0 0 2.37 KB
master RunWafRealisticBenchmark net472 218μs 126ns 486ns 0.326 0 0 2.43 KB
master RunWafRealisticBenchmarkWithAttack net6.0 123μs 90.4ns 350ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 133μs 231ns 893ns 0 0 0 1.45 KB
master RunWafRealisticBenchmarkWithAttack net472 144μs 174ns 675ns 0.216 0 0 1.48 KB
#5587 RunWafRealisticBenchmark net6.0 186μs 138ns 535ns 0 0 0 2.42 KB
#5587 RunWafRealisticBenchmark netcoreapp3.1 198μs 128ns 498ns 0 0 0 2.37 KB
#5587 RunWafRealisticBenchmark net472 216μs 43.1ns 167ns 0.324 0 0 2.43 KB
#5587 RunWafRealisticBenchmarkWithAttack net6.0 122μs 191ns 741ns 0 0 0 1.46 KB
#5587 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 131μs 102ns 395ns 0 0 0 1.45 KB
#5587 RunWafRealisticBenchmarkWithAttack net472 142μs 55.3ns 207ns 0.213 0 0 1.48 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Faster 🎉 More allocations ⚠️

Faster 🎉 in #5587

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 1.171 45,600.00 38,950.00
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 1.160 332,100.00 286,400.00 bimodal

More allocations ⚠️ in #5587

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 56.65 KB 59.12 KB 2.47 KB 4.36%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 252.13 KB 253.88 KB 1.75 KB 0.69%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 60.4μs 730ns 7.27μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 65.4μs 908ns 9.03μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 46.5μs 367ns 3.58μs 0 0 0 56.65 KB
master StringConcatAspectBenchmark net6.0 336μs 1.86μs 13.5μs 0 0 0 256.05 KB
master StringConcatAspectBenchmark netcoreapp3.1 342μs 3.08μs 29.5μs 0 0 0 252.13 KB
master StringConcatAspectBenchmark net472 332μs 7.22μs 69.6μs 0 0 0 278.53 KB
#5587 StringConcatBenchmark net6.0 61.2μs 876ns 8.72μs 0 0 0 43.44 KB
#5587 StringConcatBenchmark netcoreapp3.1 60.6μs 703ns 6.96μs 0 0 0 42.64 KB
#5587 StringConcatBenchmark net472 38.8μs 166ns 621ns 0 0 0 59.12 KB
#5587 StringConcatAspectBenchmark net6.0 280μs 6.26μs 61.7μs 0 0 0 255.93 KB
#5587 StringConcatAspectBenchmark netcoreapp3.1 329μs 2.47μs 24μs 0 0 0 253.88 KB
#5587 StringConcatAspectBenchmark net472 292μs 4.94μs 47.6μs 0 0 0 278.53 KB

anna-git
anna-git reviewed May 21, 2024
View reviewed changes
Comment thread tracer/src/Datadog.Trace/AppSec/WafEncoding/Encoder.cs Outdated
anna-git
anna-git reviewed May 21, 2024
View reviewed changes
Comment thread tracer/src/Datadog.Trace/AppSec/WafEncoding/Encoder.cs Outdated
anna-git
anna-git reviewed May 21, 2024
View reviewed changes
Comment thread tracer/src/Datadog.Trace/AppSec/WafEncoding/Encoder.cs
@dromanol
dromanol force-pushed the dani/asm/encoder_readability_refactor branch 2 times, most recently from 985700d to aa3cae1 Compare May 27, 2024 15:43
@dromanol
dromanol requested a review from anna-git May 31, 2024 08:20
anna-git
anna-git reviewed May 31, 2024
View reviewed changes
Comment thread tracer/src/Datadog.Trace/AppSec/WafEncoding/Encoder.cs Outdated
anna-git
anna-git approved these changes May 31, 2024
View reviewed changes

@anna-git anna-git 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! just the last comment on using the parameter pool if it's supplied

@dromanol
dromanol merged commit cd8153b into master Jun 6, 2024
@dromanol
dromanol deleted the dani/asm/encoder_readability_refactor branch June 6, 2024 06:54
@github-actions github-actions Bot added this to the vNext-v2 milestone Jun 6, 2024
@andrewlock andrewlock mentioned this pull request May 12, 2026
Merged
andrewlock added a commit that referenced this pull request May 26, 2026
@andrewlock
Fix WAF Encoder child miscount bug (#8617)
ee77554 
## Summary of changes

Fixes a miscount issue with the WAF encoder

## Reason for change

#5587 refactored the WAF encoder code, but there's a bug.
`ProcessKeyValuePairs` calculates `childrenCount` once, calls
`EnumerateIDictionaryItems`, and then sets `NbEntries` using the
unmodified `childrenCount`. `EnumerateIDictionaryItems` skips empty
keys, and decrements `childrenCount`, but it's working on a _local_ copy
of the value, so `ProcessKeyValuePairs` doesn't see the change, meaning
it sends too high a value in `ddWafObjectMap.NbEntries`

## Implementation details

Pass `childrenCount` by `ref`, so that decrementing it correctly
decrements the "global" value.

## Test coverage

Added some unit tests that demonstrate the issue. These caused errors in
the LegacyEncoder paths which similarly were addressed. The change in
the snapshots is due to now skipping empty keys

## Other details

This was 🤖 identified - I find this code pretty hard to follow, so
please do sense check that the change _actually_ makes sense 😅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertpi robertpi robertpi approved these changes
@anna-git anna-git anna-git approved these changes

Assignees

No one assigned

Labels

area:asm

Projects

None yet

Milestone

2.53.0

Development

Successfully merging this pull request may close these issues.

4 participants

@dromanol @andrewlock @robertpi @anna-git