Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ASM] Fix our legacy encoder benchmarks memory leak #5308

Merged
merged 13 commits into from
Mar 28, 2024
Merged

[ASM] Fix our legacy encoder benchmarks memory leak #5308

merged 13 commits into from
Mar 28, 2024

Conversation

anna-git
Copy link
Contributor

@anna-git anna-git commented Mar 14, 2024 •
edited
Loading

Summary of changes

Remove all allocations of memory and create DdWafObjectStruct on the stack.
Don't configure a free function to execute on context destroy and call the waf free function ourselves, only on parent

Reason for change

Implementation details

Test coverage

Other details

@anna-git anna-git requested a review from a team as a code owner March 14, 2024 14:26
@github-actions github-actions bot added area:tests unit tests, integration tests area:asm labels Mar 14, 2024
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Mar 14, 2024 •
edited
Loading

Datadog Report

Branch report: anna/asm/activate-encoder-benchmark
Commit report: bb6eb68
Test service: dd-trace-dotnet

✅ 0 Failed, 330073 Passed, 1582 Skipped, 48m 9.64s Wall Time
❄️ 2 New Flaky
⌛ 1 Performance Regression

New Flaky Tests (2)

  • WriteDebugLogArchiveToStream_OnlyIncludesMostRecentLogsWhenOverSizeLimit - Datadog.Trace.Tests.Logging.TracerFlare.DebugLogReaderTests - Last Failure

    Expand for error 
     Expected File.Exists(extractedPath) to be true, but found False.

  • ObfuscateWithDefaultPattern - Datadog.Trace.Tests.Util.Http.EvidenceRedactorTests - Last Failure

    Expand for error 
     Expected string to be "key1=val1&<redacted>&key2=val2" with a length of 30, but "" has a length of 0, differs near "" (index 0).

⌛ Performance Regressions vs Default Branch (1)

  • Baseline - Samples.HttpMessageHandler.windows.netcoreapp31.json.scenarios 288.26ms (+17.1ms, +6%) - Details

@andrewlock
Copy link
Member

andrewlock commented Mar 14, 2024 •
edited
Loading

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 (5308) - mean (74ms)  : 66, 82
     .   : milestone, 74,
    master - mean (76ms)  : 64, 87
     .   : milestone, 76,

    section CallTarget+Inlining+NGEN
    This PR (5308) - mean (1,002ms)  : 976, 1029
     .   : milestone, 1002,
    master - mean (1,002ms)  : 981, 1022
     .   : milestone, 1002,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5308) - mean (110ms)  : 107, 112
     .   : milestone, 110,
    master - mean (110ms)  : 107, 112
     .   : milestone, 110,

    section CallTarget+Inlining+NGEN
    This PR (5308) - mean (718ms)  : 696, 739
     .   : milestone, 718,
    master - mean (718ms)  : 694, 741
     .   : milestone, 718,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5308) - mean (94ms)  : 90, 98
     .   : milestone, 94,
    master - mean (95ms)  : 91, 99
     .   : milestone, 95,

    section CallTarget+Inlining+NGEN
    This PR (5308) - mean (678ms)  : 658, 697
     .   : milestone, 678,
    master - mean (676ms)  : 653, 698
     .   : milestone, 676,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5308) - mean (188ms)  : 186, 191
     .   : milestone, 188,
    master - mean (188ms)  : 184, 192
     .   : milestone, 188,

    section CallTarget+Inlining+NGEN
    This PR (5308) - mean (1,079ms)  : 1056, 1103
     .   : milestone, 1079,
    master - mean (1,080ms)  : 1058, 1102
     .   : milestone, 1080,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5308) - mean (288ms)  : 278, 298
     .   : milestone, 288,
    master - mean (272ms)  : 268, 276
     .   : milestone, 272,

    section CallTarget+Inlining+NGEN
    This PR (5308) - mean (874ms)  : 844, 904
     .   : milestone, 874,
    master - mean (874ms)  : 853, 895
     .   : milestone, 874,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5308) - mean (261ms)  : 255, 267
     .   : milestone, 261,
    master - mean (260ms)  : 254, 265
     .   : milestone, 260,

    section CallTarget+Inlining+NGEN
    This PR (5308) - mean (852ms)  : 828, 876
     .   : milestone, 852,
    master - mean (850ms)  : 827, 872
     .   : milestone, 850,
Loading

@andrewlock
Copy link
Member

andrewlock commented Mar 14, 2024 •
edited
Loading

Benchmarks Report for tracer 🐌

Benchmarks for #5308 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.117
  • 1 benchmarks are slower, with geometric mean 1.122
  • 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.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.82μs 45.3ns 217ns 0.0265 0.0132 0 7.51 KB
master StartStopWithChild netcoreapp3.1 10.9μs 57.9ns 301ns 0.0328 0.0164 0 7.6 KB
master StartStopWithChild net472 16.7μs 76.8ns 307ns 1.36 0.35 0.117 8.05 KB
#5308 StartStopWithChild net6.0 8.7μs 48.3ns 294ns 0.021 0.00841 0 7.51 KB
#5308 StartStopWithChild netcoreapp3.1 10.6μs 59.6ns 400ns 0.0351 0.015 0 7.6 KB
#5308 StartStopWithChild net472 16.8μs 67.4ns 261ns 1.34 0.325 0.108 8.06 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 457μs 549ns 2.13μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 595μs 164ns 612ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 827μs 4.15μs 18.1μs 0.406 0 0 3.3 KB
#5308 WriteAndFlushEnrichedTraces net6.0 458μs 127ns 492ns 0 0 0 2.7 KB
#5308 WriteAndFlushEnrichedTraces netcoreapp3.1 613μs 184ns 714ns 0 0 0 2.7 KB
#5308 WriteAndFlushEnrichedTraces net472 818μs 268ns 1.04μs 0.406 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 110ns 412ns 0.17 0 0 18.33 KB
master SendRequest netcoreapp3.1 193μs 242ns 937ns 0.191 0 0 20.49 KB
master SendRequest net472 0.00106ns 0.000248ns 0.000893ns 0 0 0 0 b
#5308 SendRequest net6.0 169μs 236ns 915ns 0.254 0 0 18.33 KB
#5308 SendRequest netcoreapp3.1 194μs 306ns 1.19μs 0.194 0 0 20.49 KB
#5308 SendRequest net472 6.91E‑05ns 6.91E‑05ns 0.000249ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 564μs 2.63μs 10.2μs 0.563 0 0 41.59 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 652μs 540ns 1.95μs 0.326 0 0 41.71 KB
master WriteAndFlushEnrichedTraces net472 873μs 4.27μs 18.1μs 8.19 2.59 0.431 53.26 KB
#5308 WriteAndFlushEnrichedTraces net6.0 553μs 350ns 1.36μs 0.551 0 0 41.73 KB
#5308 WriteAndFlushEnrichedTraces netcoreapp3.1 645μs 1.32μs 5.11μs 0.319 0 0 41.64 KB
#5308 WriteAndFlushEnrichedTraces net472 892μs 3.11μs 12μs 8.42 2.66 0.443 53.23 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 net6.0 1.2μs 0.746ns 2.79ns 0.0108 0 0 784 B
master ExecuteNonQuery netcoreapp3.1 1.59μs 0.482ns 1.87ns 0.0105 0 0 784 B
master ExecuteNonQuery net472 1.92μs 1.63ns 5.89ns 0.118 0 0 746 B
#5308 ExecuteNonQuery net6.0 1.12μs 0.544ns 1.96ns 0.0111 0 0 784 B
#5308 ExecuteNonQuery netcoreapp3.1 1.55μs 0.972ns 3.77ns 0.0109 0 0 784 B
#5308 ExecuteNonQuery net472 1.79μs 1.05ns 4.06ns 0.118 0 0 746 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.34μs 0.371ns 1.39ns 0.0141 0 0 1.01 KB
master CallElasticsearch netcoreapp3.1 1.58μs 1.06ns 3.98ns 0.0134 0 0 1.01 KB
master CallElasticsearch net472 2.42μs 0.888ns 3.32ns 0.162 0 0 1.02 KB
master CallElasticsearchAsync net6.0 1.36μs 0.342ns 1.23ns 0.0136 0 0 984 B
master CallElasticsearchAsync netcoreapp3.1 1.7μs 1.4ns 5.42ns 0.0145 0 0 1.06 KB
master CallElasticsearchAsync net472 2.65μs 1.8ns 6.97ns 0.171 0 0 1.08 KB
#5308 CallElasticsearch net6.0 1.37μs 0.856ns 3.2ns 0.0137 0 0 1.01 KB
#5308 CallElasticsearch netcoreapp3.1 1.62μs 0.924ns 3.58ns 0.0138 0 0 1.01 KB
#5308 CallElasticsearch net472 2.48μs 1.15ns 4.32ns 0.162 0 0 1.02 KB
#5308 CallElasticsearchAsync net6.0 1.35μs 0.54ns 2.02ns 0.0136 0 0 984 B
#5308 CallElasticsearchAsync netcoreapp3.1 1.73μs 1.23ns 4.61ns 0.0139 0 0 1.06 KB
#5308 CallElasticsearchAsync net472 2.66μs 1.29ns 4.66ns 0.171 0 0 1.08 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.47μs 0.551ns 2.06ns 0.0132 0 0 928 B
master ExecuteAsync netcoreapp3.1 1.68μs 0.981ns 3.8ns 0.0126 0 0 928 B
master ExecuteAsync net472 1.94μs 1.56ns 6.05ns 0.141 0 0 891 B
#5308 ExecuteAsync net6.0 1.48μs 1.76ns 6.83ns 0.0131 0 0 928 B
#5308 ExecuteAsync netcoreapp3.1 1.7μs 1.3ns 5.03ns 0.0128 0 0 928 B
#5308 ExecuteAsync net472 1.94μs 0.869ns 3.37ns 0.141 0 0 891 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.3μs 3.67ns 13.7ns 0.0302 0 0 2.16 KB
master SendAsync netcoreapp3.1 4.99μs 1.73ns 6.23ns 0.0357 0 0 2.7 KB
master SendAsync net472 7.61μs 4.09ns 14.7ns 0.483 0 0 3.05 KB
#5308 SendAsync net6.0 4.15μs 3.68ns 14.2ns 0.0291 0 0 2.16 KB
#5308 SendAsync netcoreapp3.1 4.94μs 4.02ns 14.5ns 0.0371 0 0 2.7 KB
#5308 SendAsync net472 7.74μs 4.27ns 16.5ns 0.484 0 0 3.05 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.471ns 1.7ns 0.023 0 0 1.65 KB
master EnrichedLog netcoreapp3.1 2.22μs 1.86ns 7.19ns 0.0225 0 0 1.65 KB
master EnrichedLog net472 2.59μs 1.28ns 4.8ns 0.25 0 0 1.57 KB
#5308 EnrichedLog net6.0 1.45μs 0.61ns 2.28ns 0.0235 0 0 1.65 KB
#5308 EnrichedLog netcoreapp3.1 2.32μs 1.65ns 6.17ns 0.0219 0 0 1.65 KB
#5308 EnrichedLog net472 2.58μs 1.18ns 4.42ns 0.25 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 111μs 84.1ns 326ns 0.0556 0 0 4.23 KB
master EnrichedLog netcoreapp3.1 117μs 166ns 642ns 0.0582 0 0 4.23 KB
master EnrichedLog net472 146μs 81.6ns 316ns 0.657 0.219 0 4.41 KB
#5308 EnrichedLog net6.0 112μs 76.2ns 275ns 0.056 0 0 4.23 KB
#5308 EnrichedLog netcoreapp3.1 116μs 171ns 663ns 0 0 0 4.23 KB
#5308 EnrichedLog net472 148μs 118ns 456ns 0.66 0.22 0 4.41 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 1.06ns 3.83ns 0.0309 0 0 2.21 KB
master EnrichedLog netcoreapp3.1 4.23μs 2.98ns 11.5ns 0.0301 0 0 2.21 KB
master EnrichedLog net472 5μs 2.24ns 8.38ns 0.319 0 0 2.02 KB
#5308 EnrichedLog net6.0 3.18μs 2.04ns 7.91ns 0.0301 0 0 2.21 KB
#5308 EnrichedLog netcoreapp3.1 4.03μs 1.91ns 7.14ns 0.0304 0 0 2.21 KB
#5308 EnrichedLog net472 4.82μs 2.12ns 8.19ns 0.32 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.4μs 0.836ns 3.24ns 0.0168 0 0 1.18 KB
master SendReceive netcoreapp3.1 1.82μs 0.581ns 2.17ns 0.0155 0 0 1.18 KB
master SendReceive net472 2.27μs 1.87ns 7ns 0.187 0.00114 0 1.18 KB
#5308 SendReceive net6.0 1.42μs 0.441ns 1.59ns 0.0164 0 0 1.18 KB
#5308 SendReceive netcoreapp3.1 1.87μs 0.977ns 3.65ns 0.016 0 0 1.18 KB
#5308 SendReceive net472 2.26μs 0.803ns 3.11ns 0.187 0 0 1.18 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.73μs 1.07ns 3.85ns 0.0221 0 0 1.55 KB
master EnrichedLog netcoreapp3.1 3.97μs 2.08ns 8.07ns 0.0219 0 0 1.6 KB
master EnrichedLog net472 4.47μs 3.26ns 12.2ns 0.314 0 0 1.99 KB
#5308 EnrichedLog net6.0 2.82μs 1.29ns 4.98ns 0.0211 0 0 1.55 KB
#5308 EnrichedLog netcoreapp3.1 3.97μs 1.08ns 4.2ns 0.0219 0 0 1.6 KB
#5308 EnrichedLog net472 4.42μs 1.25ns 4.31ns 0.314 0 0 1.99 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5308

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 1.122 639.01 716.88

Faster 🎉 in #5308

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 1.117 662.93 593.73

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 477ns 0.588ns 2.2ns 0.00765 0 0 552 B
master StartFinishSpan netcoreapp3.1 639ns 0.885ns 3.43ns 0.00757 0 0 552 B
master StartFinishSpan net472 781ns 1.37ns 5.31ns 0.088 0 0 554 B
master StartFinishScope net6.0 662ns 1.04ns 4.03ns 0.00954 0 0 672 B
master StartFinishScope netcoreapp3.1 922ns 1.18ns 4.42ns 0.00926 0 0 672 B
master StartFinishScope net472 940ns 0.797ns 3.09ns 0.101 0 0 634 B
#5308 StartFinishSpan net6.0 474ns 0.691ns 2.68ns 0.00762 0 0 552 B
#5308 StartFinishSpan netcoreapp3.1 718ns 0.982ns 3.8ns 0.0077 0 0 552 B
#5308 StartFinishSpan net472 790ns 0.993ns 3.85ns 0.0876 0 0 554 B
#5308 StartFinishScope net6.0 593ns 0.438ns 1.69ns 0.00937 0 0 672 B
#5308 StartFinishScope netcoreapp3.1 830ns 0.981ns 3.8ns 0.00896 0 0 672 B
#5308 StartFinishScope net472 947ns 1.23ns 4.62ns 0.1 0 0 634 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 678ns 1.16ns 4.5ns 0.00923 0 0 672 B
master RunOnMethodBegin netcoreapp3.1 980ns 1.17ns 4.55ns 0.00932 0 0 672 B
master RunOnMethodBegin net472 1.07μs 1.15ns 4.44ns 0.1 0 0 634 B
#5308 RunOnMethodBegin net6.0 682ns 0.854ns 3.31ns 0.0094 0 0 672 B
#5308 RunOnMethodBegin netcoreapp3.1 991ns 1.49ns 5.77ns 0.00916 0 0 672 B
#5308 RunOnMethodBegin net472 1.09μs 1.47ns 5.29ns 0.1 0 0 634 B

@anna-git anna-git marked this pull request as draft March 14, 2024 16:34
@anna-git anna-git force-pushed the anna/asm/activate-encoder-benchmark branch 4 times, most recently from 6f768e7 to 2db40cb Compare March 22, 2024 09:52
@andrewlock
Copy link
Member

andrewlock commented Mar 22, 2024 •
edited
Loading

Benchmarks Report for appsec 🐌

Benchmarks for #5308 compared to master:

  • 1 benchmarks are slower, with geometric mean 1.123
  • 7 benchmarks have fewer allocations
  • 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.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5308

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 1.123 205.66 231.05

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 71.3μs 85.6ns 332ns 0.0711 0 0 5.97 KB
master AllCycleSimpleBody netcoreapp3.1 61.9μs 97.2ns 377ns 0.0926 0 0 6.92 KB
master AllCycleSimpleBody net472 48.2μs 51.5ns 193ns 1.3 0 0 8.3 KB
master AllCycleMoreComplexBody net6.0 76.6μs 142ns 550ns 0.113 0 0 9.48 KB
master AllCycleMoreComplexBody netcoreapp3.1 67.7μs 81.8ns 317ns 0.136 0 0 10.33 KB
master AllCycleMoreComplexBody net472 55.4μs 35.4ns 128ns 1.86 0.0278 0 11.82 KB
master ObjectExtractorSimpleBody net6.0 145ns 0.119ns 0.46ns 0.00397 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 206ns 0.156ns 0.602ns 0.00373 0 0 272 B
master ObjectExtractorSimpleBody net472 160ns 0.156ns 0.606ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.02μs 2ns 7.49ns 0.0532 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 4.49μs 2.22ns 8.32ns 0.0494 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.75μs 2.13ns 8.26ns 0.603 0.00563 0 3.8 KB
#5308 AllCycleSimpleBody net6.0 69.8μs 49.9ns 193ns 0.0697 0 0 5.97 KB
#5308 AllCycleSimpleBody netcoreapp3.1 60.9μs 64.5ns 250ns 0.0904 0 0 6.92 KB
#5308 AllCycleSimpleBody net472 47.8μs 29.1ns 109ns 1.3 0 0 8.3 KB
#5308 AllCycleMoreComplexBody net6.0 76.7μs 94.4ns 366ns 0.116 0 0 9.48 KB
#5308 AllCycleMoreComplexBody netcoreapp3.1 70.1μs 101ns 379ns 0.137 0 0 10.33 KB
#5308 AllCycleMoreComplexBody net472 55.5μs 60.4ns 234ns 1.86 0.0277 0 11.82 KB
#5308 ObjectExtractorSimpleBody net6.0 141ns 0.0904ns 0.338ns 0.00395 0 0 280 B
#5308 ObjectExtractorSimpleBody netcoreapp3.1 231ns 0.167ns 0.624ns 0.00372 0 0 272 B
#5308 ObjectExtractorSimpleBody net472 164ns 0.153ns 0.593ns 0.0446 0 0 281 B
#5308 ObjectExtractorMoreComplexBody net6.0 3.02μs 2.62ns 10.2ns 0.0533 0 0 3.78 KB
#5308 ObjectExtractorMoreComplexBody netcoreapp3.1 4.06μs 2.01ns 7.79ns 0.0486 0 0 3.69 KB
#5308 ObjectExtractorMoreComplexBody net472 3.82μs 4.84ns 18.8ns 0.601 0.00575 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
#5308 EncodeArgs net6.0 39.9μs 43.2ns 167ns 0.456 0 0 32.4 KB
#5308 EncodeArgs netcoreapp3.1 56.1μs 25ns 93.7ns 0.445 0 0 32.4 KB
#5308 EncodeArgs net472 69.5μs 34.6ns 134ns 5.14 0.0695 0 32.5 KB
#5308 EncodeLegacyArgs net6.0 71.7μs 33.8ns 131ns 0 0 0 2.14 KB
#5308 EncodeLegacyArgs netcoreapp3.1 104μs 174ns 651ns 0 0 0 2.14 KB
#5308 EncodeLegacyArgs net472 158μs 85.3ns 330ns 0.315 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #5308

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑net472 4.19 KB 1.39 KB -2.8 KB -66.87%
Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑net6.0 4.15 KB 1.37 KB -2.78 KB -67.05%
Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑net472 6.59 KB 2.17 KB -4.42 KB -67.12%
Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑netcoreapp3.1 4.14 KB 1.36 KB -2.78 KB -67.18%
Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑net6.0 6.51 KB 2.13 KB -4.39 KB -67.33%
Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑netcoreapp3.1 6.49 KB 2.1 KB -4.38 KB -67.57%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 191μs 782ns 3.03μs 0 0 0 6.51 KB
master RunWafRealisticBenchmark netcoreapp3.1 203μs 194ns 750ns 0 0 0 6.49 KB
master RunWafRealisticBenchmark net472 223μs 107ns 415ns 0.997 0 0 6.59 KB
master RunWafRealisticBenchmarkWithAttack net6.0 125μs 149ns 576ns 0 0 0 4.15 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 134μs 187ns 725ns 0 0 0 4.14 KB
master RunWafRealisticBenchmarkWithAttack net472 148μs 296ns 1.15μs 0.657 0 0 4.19 KB
#5308 RunWafRealisticBenchmark net6.0 182μs 290ns 1.12μs 0 0 0 2.13 KB
#5308 RunWafRealisticBenchmark netcoreapp3.1 195μs 166ns 622ns 0 0 0 2.1 KB
#5308 RunWafRealisticBenchmark net472 217μs 136ns 525ns 0.322 0 0 2.17 KB
#5308 RunWafRealisticBenchmarkWithAttack net6.0 122μs 36ns 135ns 0 0 0 1.37 KB
#5308 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 137ns 531ns 0 0 0 1.36 KB
#5308 RunWafRealisticBenchmarkWithAttack net472 142μs 65ns 243ns 0.212 0 0 1.39 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5308

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 201.19 KB 209.92 KB 8.73 KB 4.34%

Fewer allocations 🎉 in #5308

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 61.7 KB 57.9 KB -3.81 KB -6.17%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 53.8μs 192ns 1.29μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 62.7μs 777ns 7.73μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.1μs 174ns 675ns 0 0 0 61.7 KB
master StringConcatAspectBenchmark net6.0 284μs 1.62μs 11.2μs 0 0 0 201.19 KB
master StringConcatAspectBenchmark netcoreapp3.1 316μs 1.74μs 10.2μs 0 0 0 203.18 KB
master StringConcatAspectBenchmark net472 256μs 4.58μs 43.7μs 0 0 0 221.18 KB
#5308 StringConcatBenchmark net6.0 52.5μs 183ns 686ns 0 0 0 43.44 KB
#5308 StringConcatBenchmark netcoreapp3.1 64.8μs 790ns 7.7μs 0 0 0 42.64 KB
#5308 StringConcatBenchmark net472 39.2μs 198ns 906ns 0 0 0 57.9 KB
#5308 StringConcatAspectBenchmark net6.0 289μs 5.93μs 57.5μs 0 0 0 209.92 KB
#5308 StringConcatAspectBenchmark netcoreapp3.1 311μs 1.73μs 11.1μs 0 0 0 202.29 KB
#5308 StringConcatAspectBenchmark net472 254μs 3.28μs 31μs 0 0 0 221.18 KB

@anna-git anna-git force-pushed the anna/asm/activate-encoder-benchmark branch from 648b393 to 6307468 Compare March 24, 2024 19:36
@anna-git anna-git marked this pull request as ready for review March 25, 2024 08:55
@anna-git anna-git requested a review from a team as a code owner March 25, 2024 08:55
@anna-git anna-git changed the title [ASM] Activate the encoder benchmarks again [ASM] Fix our legacy encoder benchmarks memory leak Mar 25, 2024
@anna-git anna-git force-pushed the anna/asm/activate-encoder-benchmark branch from 82f0455 to babe539 Compare March 25, 2024 12:30
kevingosse
kevingosse approved these changes Mar 27, 2024
View reviewed changes
tracer/src/Datadog.Trace/AppSec/Waf/NativeBindings/WafLibraryInvoker.cs Outdated
private readonly IntPtr _freeObjectFuncField;
private readonly SetupLoggingDelegate _setupLogging;
private readonly SetupLogCallbackDelegate _setupLogCallbackField;
private readonly UpdateDelegate _updateField;
private string _version = null;
internal static int SizeOfDdWafObject = Marshal.SizeOf(typeof(DdwafObjectStruct));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes removed in 7ae7027

tracer/src/Datadog.Trace/AppSec/Waf/Initialization/WafConfigurator.cs Outdated
@@ -112,9 +112,9 @@ private static void LogRuleDetailsIfDebugEnabled(JToken root)
return root;
}

internal InitResult Configure(IntPtr rulesObj, IEncoder encoder, DdwafConfigStruct configStruct, ref DdwafObjectStruct diagnostics, string? rulesFile)
internal InitResult Configure(DdwafObjectStruct rulesObj, IEncoder encoder, DdwafConfigStruct configStruct, ref DdwafObjectStruct diagnostics, string? rulesFile)
Copy link
Collaborator

@kevingosse kevingosse Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NIT: You could change it to ref DdwafObjectStruct rulesObj to avoid the copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes changed in 7ae7027

tracer/src/Datadog.Trace/AppSec/Waf/NativeBindings/WafLibraryInvoker.cs Outdated

internal void ObjectFreePtr(ref IntPtr input)
internal void ObjectFreePtr(IntPtr input)
Copy link
Collaborator

@kevingosse kevingosse Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You could make it ObjectFreePtr(ref DdwafObjectStruct input); to simplify it the same way as you did for Result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes changed in 7ae7027
It makes me do tricks in Obj as I have to:

 if (_handle is not null && _handle.Value.Target is not null)
            {
                var item = (DdwafObjectStruct)_handle.Value.Target;
                wafLibraryInvoker.ObjectFree(ref item);
                _handle.Value.Free();
            }

but it works :)

nb: the ddwafobject struct tree is not always disposed on the stack, it's disposed later at the end of the request's lifecycle

tracer/src/Datadog.Trace/AppSec/Waf/Waf.cs
{
UpdateResult? res = null;
DdwafObjectStruct? diagnostics = null;
var diagnosticsValue = new DdwafObjectStruct { Type = DDWAF_OBJ_TYPE.DDWAF_OBJ_MAP };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't you call ObjectMap()? 🤔

Copy link
Contributor Author

@anna-git anna-git Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the objects is filled by the update function called on the waf as well as the creation function one and it's all good when we dispose it calling the ObjectFree function from the waf on it so I guess it's all good and avoids a p'invoke like this ..

tracer/src/Datadog.Trace/AppSec/WafEncoding/EncoderLegacy.cs Outdated
Comment on lines 38 to 47
DDWAF_OBJ_TYPE.DDWAF_OBJ_INVALID => ObjType.Invalid,
DDWAF_OBJ_TYPE.DDWAF_OBJ_SIGNED => ObjType.SignedNumber,
DDWAF_OBJ_TYPE.DDWAF_OBJ_UNSIGNED => ObjType.UnsignedNumber,
DDWAF_OBJ_TYPE.DDWAF_OBJ_STRING => ObjType.String,
DDWAF_OBJ_TYPE.DDWAF_OBJ_BOOL => ObjType.Bool,
DDWAF_OBJ_TYPE.DDWAF_OBJ_DOUBLE => ObjType.Double,
DDWAF_OBJ_TYPE.DDWAF_OBJ_ARRAY => ObjType.Array,
DDWAF_OBJ_TYPE.DDWAF_OBJ_MAP => ObjType.Map,
DDWAF_OBJ_TYPE.DDWAF_OBJ_NULL => ObjType.Null,
_ => throw new Exception($"Invalid DDWAF_INPUT_TYPE {t}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's a 1:1 mapping between the values, you could just do return (ObjType)t;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed in 7ae7027

tracer/src/Datadog.Trace/AppSec/WafEncoding/Obj.cs Outdated
using Datadog.Trace.AppSec.Waf.NativeBindings;

namespace Datadog.Trace.AppSec.WafEncoding
{
// NOTE: this is referred to as ddwaf_object in the C++ code, we call it Obj to avoid a naming clash
internal class Obj : IDisposable
internal class Obj
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this class still needed? As long as DdwafObjectStruct is on the stack, it doesn't need to be pinned. I feel like you should be able to directly return DdwafObjectStruct where you currently use Obj

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but we still need to pin the parent DdWafObjectStruct to dispose its reference later :/ as it's not always disposed within the method RunWaf, the list of Obj build up within the Context and need to be disposed only at the end of the request when the context is destroyed... so we need to pin them somewhere.. 🤔

daniel-romano-DD
daniel-romano-DD approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@daniel-romano-DD daniel-romano-DD left a comment

Choose a reason for hiding this comment

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

Really nice work

robertpi
robertpi approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@robertpi robertpi left a comment

Choose a reason for hiding this comment

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

Great work, many thanks!

@anna-git anna-git force-pushed the anna/asm/activate-encoder-benchmark branch from f27ee3d to bb6eb68 Compare March 28, 2024 16:19
@anna-git anna-git merged commit b5c3d4e into master Mar 28, 2024
54 of 57 checks passed
@anna-git anna-git deleted the anna/asm/activate-encoder-benchmark branch March 28, 2024 18:27
@github-actions github-actions bot added this to the vNext milestone Mar 28, 2024
@andrewlock andrewlock modified the milestones: vNext, vNext-v2 Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-romano-DD daniel-romano-DD daniel-romano-DD approved these changes

@kevingosse kevingosse kevingosse approved these changes

@robertpi robertpi robertpi approved these changes

Assignees
No one assigned
Labels
area:asm area:tests unit tests, integration tests
Projects
None yet
Milestone
2.50.0
Development

Successfully merging this pull request may close these issues.
5 participants
@anna-git @andrewlock @robertpi @kevingosse @daniel-romano-DD