Skip to content

[ASM] Extend locking to cover _disposed member variable#6319

Merged
robertpi merged 2 commits into
masterfrom
robert/asm/extent-locking-to-cover-disposed-member-variable
Dec 2, 2024
Merged

[ASM] Extend locking to cover _disposed member variable#6319
robertpi merged 2 commits into
masterfrom
robert/asm/extent-locking-to-cover-disposed-member-variable

Conversation

@robertpi

@robertpi robertpi commented Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Summary of changes

We have seen that some clients experience crashes that seem to be caused to concurrent access to members within the WAF. As the WAF does not implemented any locking, it's client code of the WAF that should ensure there's no incorrect concurrent access to the WAF.

Test coverage

Added a unit test that tests concurrent access to the context and fails before this changes is implemented.

@datadog-ddstaging

datadog-ddstaging Bot commented Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: robert/asm/extent-locking-to-cover-disposed-member-variable
Commit report: ec59707
Test service: dd-trace-dotnet

✅ 0 Failed, 451293 Passed, 2743 Skipped, 20h 42m 41.67s Total Time
❄️ 1 New Flaky
⌛ 1 Performance Regression

New Flaky Tests (1)

  • EarlyFlakeDetection - Datadog.Trace.ClrProfiler.IntegrationTests.CI.UdsXUnitEvpTests - Last Failure

    Expand for error 
     The sample did not exit in 600000ms. Memory dump taken: True. Killing process.

⌛ Performance Regressions vs Default Branch (1)

  • Profiler_contention - scenarios 2.35s (+73.84ms, +3%) - Details

@andrewlock

andrewlock commented Nov 19, 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 (6319) - mean (69ms)  : 65, 73
     .   : milestone, 69,
    master - mean (69ms)  : 66, 71
     .   : milestone, 69,

    section CallTarget+Inlining+NGEN
    This PR (6319) - mean (983ms)  : 943, 1022
     .   : milestone, 983,
    master - mean (979ms)  : 960, 998
     .   : milestone, 979,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6319) - mean (108ms)  : 105, 110
     .   : milestone, 108,
    master - mean (108ms)  : 106, 110
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (6319) - mean (682ms)  : 664, 700
     .   : milestone, 682,
    master - mean (680ms)  : 666, 694
     .   : milestone, 680,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6319) - mean (91ms)  : 90, 93
     .   : milestone, 91,
    master - mean (91ms)  : 89, 93
     .   : milestone, 91,

    section CallTarget+Inlining+NGEN
    This PR (6319) - mean (633ms)  : 617, 649
     .   : milestone, 633,
    master - mean (633ms)  : 617, 649
     .   : milestone, 633,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6319) - mean (191ms)  : 186, 196
     .   : milestone, 191,
    master - mean (190ms)  : 186, 195
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6319) - mean (1,092ms)  : 1064, 1121
     .   : milestone, 1092,
    master - mean (1,093ms)  : 1066, 1121
     .   : milestone, 1093,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6319) - mean (276ms)  : 272, 280
     .   : milestone, 276,
    master - mean (278ms)  : 273, 282
     .   : milestone, 278,

    section CallTarget+Inlining+NGEN
    This PR (6319) - mean (875ms)  : 840, 910
     .   : milestone, 875,
    master - mean (879ms)  : 850, 908
     .   : milestone, 879,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6319) - mean (266ms)  : 262, 270
     .   : milestone, 266,
    master - mean (269ms)  : 264, 273
     .   : milestone, 269,

    section CallTarget+Inlining+NGEN
    This PR (6319) - mean (857ms)  : 824, 890
     .   : milestone, 857,
    master - mean (859ms)  : 829, 890
     .   : milestone, 859,
Loading

@andrewlock

andrewlock commented Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Throughput/Crank Report ⚡

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 (6319) (11.331M)   : 0, 11330881
    master (11.117M)   : 0, 11116942
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6319) (7.237M)   : 0, 7237407
    master (7.260M)   : 0, 7259731
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.668M)   : 0, 7667598

    section Manual
    master (11.257M)   : 0, 11257422

    section Manual + Automatic
    This PR (6319) (6.760M)   : 0, 6760316
    master (6.752M)   : 0, 6751591

    section DD_TRACE_ENABLED=0
    master (10.429M)   : 0, 10429279
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6319) (9.726M)   : 0, 9725905
    master (9.664M)   : 0, 9663866
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6319) (6.475M)   : 0, 6475202
    master (6.467M)   : 0, 6466720

    section Trace stats
    master (6.704M)   : 0, 6704260

    section Manual
    master (9.711M)   : 0, 9710712

    section Manual + Automatic
    This PR (6319) (5.848M)   : 0, 5847583
    master (5.991M)   : 0, 5991245

    section DD_TRACE_ENABLED=0
    master (8.961M)   : 0, 8961362
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6319) (9.603M)   : 0, 9602849
    master (10.283M)   : 0, 10282578
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6319) (6.229M)   : crit ,0, 6229125
    master (6.605M)   : 0, 6605152
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (7.186M)   : 0, 7186132

    section Manual
    master (10.025M)   : 0, 10025447

    section Manual + Automatic
    This PR (6319) (6.011M)   : 0, 6011246
    master (6.195M)   : 0, 6194775

    section DD_TRACE_ENABLED=0
    master (9.464M)   : 0, 9463946
Loading

@andrewlock

andrewlock commented Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report for tracer 🐌

Benchmarks for #6319 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.139
  • 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.18μs 46.2ns 330ns 0.0162 0.0081 0 5.62 KB
master StartStopWithChild netcoreapp3.1 10.3μs 47.9ns 220ns 0.0244 0.00976 0 5.8 KB
master StartStopWithChild net472 16.3μs 52ns 201ns 1.04 0.3 0.0972 6.21 KB
#6319 StartStopWithChild net6.0 8.01μs 43ns 223ns 0.0202 0.00807 0 5.61 KB
#6319 StartStopWithChild netcoreapp3.1 10.4μs 58.6ns 397ns 0.0201 0.0101 0 5.8 KB
#6319 StartStopWithChild net472 16.3μs 61.1ns 237ns 1.06 0.328 0.0959 6.21 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 500μs 486ns 1.82μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 662μs 271ns 1.01μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 850μs 664ns 2.57μs 0.422 0 0 3.3 KB
#6319 WriteAndFlushEnrichedTraces net6.0 502μs 163ns 632ns 0 0 0 2.7 KB
#6319 WriteAndFlushEnrichedTraces netcoreapp3.1 645μs 255ns 918ns 0 0 0 2.7 KB
#6319 WriteAndFlushEnrichedTraces net472 846μs 571ns 2.06μs 0.419 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 152μs 1.05μs 10.3μs 0.15 0 0 14.47 KB
master SendRequest netcoreapp3.1 170μs 1.13μs 11.2μs 0.176 0 0 17.27 KB
master SendRequest net472 0.000897ns 0.000497ns 0.00179ns 0 0 0 0 b
#6319 SendRequest net6.0 150μs 896ns 8.83μs 0.142 0 0 14.47 KB
#6319 SendRequest netcoreapp3.1 172μs 1.23μs 12.3μs 0.165 0 0 17.27 KB
#6319 SendRequest net472 0.000315ns 0.000221ns 0.000796ns 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 587μs 3.16μs 16.7μs 0.568 0 0 41.81 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 699μs 3.76μs 19.5μs 0.336 0 0 41.73 KB
master WriteAndFlushEnrichedTraces net472 862μs 3.51μs 13.6μs 8.08 2.55 0.425 53.28 KB
#6319 WriteAndFlushEnrichedTraces net6.0 566μs 2.05μs 7.65μs 0.534 0 0 41.73 KB
#6319 WriteAndFlushEnrichedTraces netcoreapp3.1 673μs 3.8μs 25.2μs 0.332 0 0 41.72 KB
#6319 WriteAndFlushEnrichedTraces net472 838μs 2.12μs 7.64μs 8.33 2.5 0.417 53.27 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.35μs 2.23ns 8.65ns 0.0142 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.77μs 1.4ns 5.42ns 0.0132 0 0 1.02 KB
master ExecuteNonQuery net472 2.13μs 1.66ns 6.44ns 0.156 0.00105 0 987 B
#6319 ExecuteNonQuery net6.0 1.3μs 1.37ns 5.32ns 0.0143 0 0 1.02 KB
#6319 ExecuteNonQuery netcoreapp3.1 1.83μs 1.13ns 4.22ns 0.0136 0 0 1.02 KB
#6319 ExecuteNonQuery net472 2.18μs 1.69ns 6.08ns 0.156 0.00108 0 987 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.18μs 0.79ns 3.06ns 0.0137 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.64μs 1.06ns 4.11ns 0.0129 0 0 976 B
master CallElasticsearch net472 2.65μs 2.85ns 11ns 0.157 0 0 995 B
master CallElasticsearchAsync net6.0 1.33μs 0.518ns 1.94ns 0.0133 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.64μs 0.619ns 2.32ns 0.0139 0 0 1.02 KB
master CallElasticsearchAsync net472 2.56μs 1.65ns 5.96ns 0.167 0 0 1.05 KB
#6319 CallElasticsearch net6.0 1.23μs 0.651ns 2.52ns 0.0137 0 0 976 B
#6319 CallElasticsearch netcoreapp3.1 1.64μs 2.06ns 7.71ns 0.013 0 0 976 B
#6319 CallElasticsearch net472 2.51μs 1.45ns 5.23ns 0.158 0 0 995 B
#6319 CallElasticsearchAsync net6.0 1.31μs 0.605ns 2.26ns 0.013 0 0 952 B
#6319 CallElasticsearchAsync netcoreapp3.1 1.67μs 1.77ns 6.62ns 0.0135 0 0 1.02 KB
#6319 CallElasticsearchAsync net472 2.62μs 1.97ns 7.61ns 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.24μs 0.602ns 2.25ns 0.013 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.63μs 0.695ns 2.69ns 0.0129 0 0 952 B
master ExecuteAsync net472 1.83μs 1.12ns 4.34ns 0.145 0 0 915 B
#6319 ExecuteAsync net6.0 1.28μs 1.3ns 5.04ns 0.0135 0 0 952 B
#6319 ExecuteAsync netcoreapp3.1 1.66μs 1.44ns 5.38ns 0.0124 0 0 952 B
#6319 ExecuteAsync net472 1.84μs 0.303ns 1.17ns 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.16μs 1.93ns 7.47ns 0.0311 0 0 2.31 KB
master SendAsync netcoreapp3.1 5.3μs 1.5ns 5.63ns 0.0371 0 0 2.85 KB
master SendAsync net472 7.26μs 1.63ns 6.12ns 0.493 0 0 3.12 KB
#6319 SendAsync net6.0 4.4μs 1.56ns 5.83ns 0.0309 0 0 2.31 KB
#6319 SendAsync netcoreapp3.1 5.54μs 1.97ns 7.63ns 0.0387 0 0 2.85 KB
#6319 SendAsync net472 7.48μs 1.63ns 6.32ns 0.494 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.59μs 1.24ns 4.65ns 0.0232 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.28μs 1.44ns 5.57ns 0.0227 0 0 1.64 KB
master EnrichedLog net472 2.6μs 1.31ns 4.89ns 0.249 0 0 1.57 KB
#6319 EnrichedLog net6.0 1.48μs 0.799ns 2.99ns 0.0226 0 0 1.64 KB
#6319 EnrichedLog netcoreapp3.1 2.19μs 1.22ns 4.57ns 0.022 0 0 1.64 KB
#6319 EnrichedLog net472 2.66μs 0.663ns 2.39ns 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 119μs 168ns 652ns 0 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 124μs 193ns 749ns 0 0 0 4.28 KB
master EnrichedLog net472 153μs 155ns 602ns 0.688 0.229 0 4.46 KB
#6319 EnrichedLog net6.0 120μs 152ns 587ns 0.06 0 0 4.28 KB
#6319 EnrichedLog netcoreapp3.1 125μs 74.4ns 268ns 0 0 0 4.28 KB
#6319 EnrichedLog net472 153μs 212ns 821ns 0.687 0.229 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.08μs 0.993ns 3.85ns 0.0309 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.12μs 2.71ns 10.5ns 0.0288 0 0 2.2 KB
master EnrichedLog net472 4.88μs 1.23ns 4.62ns 0.319 0 0 2.02 KB
#6319 EnrichedLog net6.0 3.15μs 0.92ns 3.44ns 0.03 0 0 2.2 KB
#6319 EnrichedLog netcoreapp3.1 4.25μs 1.66ns 6.43ns 0.0294 0 0 2.2 KB
#6319 EnrichedLog net472 4.88μs 1.21ns 4.71ns 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.37μs 0.621ns 2.32ns 0.0157 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.84μs 1.15ns 4.13ns 0.0157 0 0 1.14 KB
master SendReceive net472 2.1μs 1.48ns 5.73ns 0.183 0 0 1.16 KB
#6319 SendReceive net6.0 1.3μs 0.79ns 3.06ns 0.0162 0 0 1.14 KB
#6319 SendReceive netcoreapp3.1 1.82μs 1.22ns 4.74ns 0.0155 0 0 1.14 KB
#6319 SendReceive net472 1.99μs 1.44ns 5.56ns 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.85μs 1.51ns 5.85ns 0.0228 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.89μs 1.97ns 7.64ns 0.0214 0 0 1.65 KB
master EnrichedLog net472 4.33μs 3.09ns 11.6ns 0.323 0 0 2.04 KB
#6319 EnrichedLog net6.0 2.75μs 0.63ns 2.44ns 0.0221 0 0 1.6 KB
#6319 EnrichedLog netcoreapp3.1 3.74μs 2.37ns 8.56ns 0.0224 0 0 1.65 KB
#6319 EnrichedLog net472 4.41μs 3.28ns 12.7ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6319

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 1.139 458.08 402.30

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 458ns 0.228ns 0.883ns 0.00802 0 0 576 B
master StartFinishSpan netcoreapp3.1 572ns 0.241ns 0.934ns 0.0078 0 0 576 B
master StartFinishSpan net472 737ns 0.746ns 2.89ns 0.0917 0 0 578 B
master StartFinishScope net6.0 562ns 0.786ns 3.04ns 0.00967 0 0 696 B
master StartFinishScope netcoreapp3.1 740ns 0.373ns 1.44ns 0.00927 0 0 696 B
master StartFinishScope net472 879ns 1.34ns 5.18ns 0.104 0 0 658 B
#6319 StartFinishSpan net6.0 402ns 0.228ns 0.883ns 0.00797 0 0 576 B
#6319 StartFinishSpan netcoreapp3.1 594ns 0.38ns 1.42ns 0.00775 0 0 576 B
#6319 StartFinishSpan net472 672ns 0.384ns 1.49ns 0.0916 0 0 578 B
#6319 StartFinishScope net6.0 558ns 0.297ns 1.15ns 0.00979 0 0 696 B
#6319 StartFinishScope netcoreapp3.1 801ns 0.509ns 1.97ns 0.00933 0 0 696 B
#6319 StartFinishScope net472 850ns 0.429ns 1.61ns 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 693ns 0.626ns 2.43ns 0.00977 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 878ns 0.466ns 1.74ns 0.00925 0 0 696 B
master RunOnMethodBegin net472 1.15μs 1.32ns 4.77ns 0.104 0 0 658 B
#6319 RunOnMethodBegin net6.0 681ns 0.418ns 1.62ns 0.00965 0 0 696 B
#6319 RunOnMethodBegin netcoreapp3.1 899ns 0.548ns 2.05ns 0.00952 0 0 696 B
#6319 RunOnMethodBegin net472 1.15μs 0.873ns 3.38ns 0.104 0 0 658 B

@robertpi robertpi changed the title extent locking to cover _disposed member variable [ASM] Extend locking to cover _disposed member variable Nov 20, 2024
@robertpi
robertpi marked this pull request as ready for review November 20, 2024 09:46
@robertpi
robertpi requested a review from a team as a code owner November 20, 2024 09:46
dromanol
dromanol approved these changes Nov 20, 2024
View reviewed changes

@dromanol dromanol 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

@robertpi
robertpi force-pushed the robert/asm/extent-locking-to-cover-disposed-member-variable branch from a2f8567 to ec59707 Compare December 2, 2024 10:44
@andrewlock

andrewlock commented Dec 2, 2024

Copy link
Copy Markdown
Member

Benchmarks Report for appsec 🐌

Benchmarks for #6319 compared to master:

  • All benchmarks have the same speed
  • 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 - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 216μs 42.5ns 153ns 2.47 0 0 176.82 KB
master AllCycleSimpleBody netcoreapp3.1 319μs 132ns 475ns 2.42 0 0 184.27 KB
master AllCycleSimpleBody net472 279μs 121ns 451ns 33.3 2.51 0 210.04 KB
master AllCycleMoreComplexBody net6.0 225μs 154ns 556ns 2.58 0 0 180.33 KB
master AllCycleMoreComplexBody netcoreapp3.1 312μs 73ns 263ns 2.51 0 0 187.68 KB
master AllCycleMoreComplexBody net472 285μs 210ns 815ns 33.9 2.55 0 213.56 KB
master ObjectExtractorSimpleBody net6.0 147ns 0.117ns 0.439ns 0.00391 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 209ns 0.155ns 0.601ns 0.00364 0 0 272 B
master ObjectExtractorSimpleBody net472 212ns 0.175ns 0.678ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.03μs 1.99ns 7.45ns 0.0529 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 4.05μs 7.07ns 27.4ns 0.05 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 4.43μs 4.13ns 16ns 0.603 0.00663 0 3.8 KB
#6319 AllCycleSimpleBody net6.0 219μs 94.8ns 355ns 2.49 0 0 176.82 KB
#6319 AllCycleSimpleBody netcoreapp3.1 317μs 123ns 459ns 2.55 0 0 184.26 KB
#6319 AllCycleSimpleBody net472 283μs 506ns 1.9μs 33.4 2.56 0 210.04 KB
#6319 AllCycleMoreComplexBody net6.0 223μs 86.5ns 312ns 2.54 0 0 180.33 KB
#6319 AllCycleMoreComplexBody netcoreapp3.1 318μs 74.5ns 279ns 2.54 0 0 187.68 KB
#6319 AllCycleMoreComplexBody net472 294μs 236ns 913ns 33.8 2.59 0 213.56 KB
#6319 ObjectExtractorSimpleBody net6.0 139ns 0.117ns 0.439ns 0.00394 0 0 280 B
#6319 ObjectExtractorSimpleBody netcoreapp3.1 198ns 0.122ns 0.455ns 0.00372 0 0 272 B
#6319 ObjectExtractorSimpleBody net472 211ns 0.227ns 0.88ns 0.0446 0 0 281 B
#6319 ObjectExtractorMoreComplexBody net6.0 3.1μs 0.873ns 3.03ns 0.0529 0 0 3.78 KB
#6319 ObjectExtractorMoreComplexBody netcoreapp3.1 3.88μs 2.8ns 10.8ns 0.0506 0 0 3.69 KB
#6319 ObjectExtractorMoreComplexBody net472 4.43μs 1.95ns 7.31ns 0.602 0.00664 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 40.1μs 14.9ns 53.9ns 0.463 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 53.8μs 26.8ns 96.6ns 0.428 0 0 32.4 KB
master EncodeArgs net472 66.5μs 32.2ns 116ns 5.15 0.0665 0 32.5 KB
master EncodeLegacyArgs net6.0 74.9μs 120ns 463ns 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 106μs 271ns 1.05μs 0 0 0 2.14 KB
master EncodeLegacyArgs net472 156μs 72.7ns 282ns 0.311 0 0 2.15 KB
#6319 EncodeArgs net6.0 36.7μs 12.2ns 47.2ns 0.461 0 0 32.4 KB
#6319 EncodeArgs netcoreapp3.1 53.7μs 29.1ns 113ns 0.429 0 0 32.4 KB
#6319 EncodeArgs net472 67.2μs 34.6ns 129ns 5.15 0.0669 0 32.5 KB
#6319 EncodeLegacyArgs net6.0 74.3μs 33ns 123ns 0 0 0 2.14 KB
#6319 EncodeLegacyArgs netcoreapp3.1 105μs 227ns 818ns 0 0 0 2.14 KB
#6319 EncodeLegacyArgs net472 160μs 144ns 538ns 0.322 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 182μs 232ns 900ns 0 0 0 2.44 KB
master RunWafRealisticBenchmark netcoreapp3.1 196μs 282ns 1.09μs 0 0 0 2.39 KB
master RunWafRealisticBenchmark net472 207μs 68.8ns 257ns 0.31 0 0 2.46 KB
master RunWafRealisticBenchmarkWithAttack net6.0 122μs 63.7ns 238ns 0 0 0 1.47 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 159ns 595ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack net472 140μs 127ns 492ns 0.21 0 0 1.48 KB
#6319 RunWafRealisticBenchmark net6.0 182μs 192ns 666ns 0 0 0 2.44 KB
#6319 RunWafRealisticBenchmark netcoreapp3.1 195μs 116ns 449ns 0 0 0 2.39 KB
#6319 RunWafRealisticBenchmark net472 209μs 133ns 513ns 0.312 0 0 2.46 KB
#6319 RunWafRealisticBenchmarkWithAttack net6.0 122μs 120ns 464ns 0 0 0 1.47 KB
#6319 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 131μs 47.5ns 171ns 0 0 0 1.46 KB
#6319 RunWafRealisticBenchmarkWithAttack net472 140μs 72.8ns 282ns 0.21 0 0 1.48 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #6319

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 59.33 KB 59.65 KB 320 B 0.54%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 60.3μs 686ns 6.62μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 62.7μs 863ns 8.36μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.9μs 105ns 363ns 0 0 0 59.33 KB
master StringConcatAspectBenchmark net6.0 298μs 4.98μs 48.3μs 0 0 0 255.16 KB
master StringConcatAspectBenchmark netcoreapp3.1 345μs 1.97μs 14.8μs 0 0 0 253.82 KB
master StringConcatAspectBenchmark net472 289μs 6.11μs 59.5μs 0 0 0 278.53 KB
#6319 StringConcatBenchmark net6.0 62.2μs 990ns 9.85μs 0 0 0 43.44 KB
#6319 StringConcatBenchmark netcoreapp3.1 63.4μs 744ns 7.17μs 0 0 0 42.64 KB
#6319 StringConcatBenchmark net472 38.2μs 83.3ns 300ns 0 0 0 59.65 KB
#6319 StringConcatAspectBenchmark net6.0 309μs 1.67μs 8.86μs 0 0 0 254.93 KB
#6319 StringConcatAspectBenchmark netcoreapp3.1 352μs 1.8μs 9.83μs 0 0 0 254.46 KB
#6319 StringConcatAspectBenchmark net472 292μs 5.79μs 56.1μs 0 0 0 278.53 KB

@robertpi
robertpi merged commit 542aaa1 into master Dec 2, 2024
@robertpi
robertpi deleted the robert/asm/extent-locking-to-cover-disposed-member-variable branch December 2, 2024 15:12
@github-actions github-actions Bot added this to the vNext-v3 milestone Dec 2, 2024
veerbia pushed a commit that referenced this pull request Dec 16, 2024
@robertpi @veerbia
[ASM] Extend locking to cover _disposed member variable (#6319)
b414810 
## Summary of changes

We have seen that some clients experience crashes that seem to be caused
to concurrent access to members within the WAF. As the WAF does not
implemented any locking, it's client code of the WAF that should ensure
there's no incorrect concurrent access to the WAF.

## Test coverage

Added a unit test that tests concurrent access to the context and fails
before this changes is implemented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NachoEchevarria NachoEchevarria NachoEchevarria approved these changes
@dromanol dromanol dromanol approved these changes
+1 more reviewer
@kevingosse kevingosse kevingosse approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:asm identified-by:crashtracking

Projects

None yet

Milestone

3.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@robertpi @andrewlock @kevingosse @NachoEchevarria @dromanol