Skip to content

Add unchecked to FNV hash calculations#3109

Merged
andrewlock merged 1 commit into
masterfrom
andrew/fix-fnv
Aug 23, 2022
Merged

Add unchecked to FNV hash calculations#3109
andrewlock merged 1 commit into
masterfrom
andrew/fix-fnv

Conversation

@andrewlock

@andrewlock andrewlock commented Aug 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Summary of changes

Adds unchecked to the FNV hash calculations

Reason for change

We should be doing unchecked calculations (so that we quietly overflow).

Implementation details

Added unchecked around the calculations. Technically I don't think these are necessary because

Test coverage

Couldn't find a good way to test it, but all the existing tests pass

Other details

Thanks for flagging @kevingosse

@andrewlock
andrewlock requested a review from a team as a code owner August 22, 2022 12:19
@github-actions github-actions Bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Aug 22, 2022
@andrewlock

andrewlock commented Aug 22, 2022

Copy link
Copy Markdown
Member Author

Benchmarks Report 🐌

Benchmarks for #3109 compared to master:

  • 2 benchmarks are slower, with geometric mean 1.216
  • 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 739μs 265ns 991ns 0.37 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 456μs 144ns 540ns 0 0 0 2.58 KB
#3109 WriteAndFlushEnrichedTraces net472 727μs 317ns 1.19μs 0.361 0 0 3.18 KB
#3109 WriteAndFlushEnrichedTraces netcoreapp3.1 456μs 245ns 950ns 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #3109

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleSimpleBody‑net472 1.263 182.09 229.98
Benchmarks.Trace.AppSecBodyBenchmark.BodyExtractorSimpleBody‑netcoreapp3.1 1.171 218.07 255.43

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 182ns 0.145ns 0.524ns 0.0675 9.2E-05 0 425 B
master AllCycleSimpleBody netcoreapp3.1 236ns 0.212ns 0.792ns 0.00577 0 0 424 B
master AllCycleMoreComplexBody net472 182ns 0.791ns 3.06ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 237ns 0.264ns 1.02ns 0.00545 0 0 400 B
master BodyExtractorSimpleBody net472 261ns 0.238ns 0.92ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 218ns 0.294ns 1.14ns 0.00373 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.4μs 7.14ns 26.7ns 1.21 0.0215 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.4μs 14.3ns 53.6ns 0.0928 0 0 6.75 KB
#3109 AllCycleSimpleBody net472 230ns 0.211ns 0.816ns 0.0675 0 0 425 B
#3109 AllCycleSimpleBody netcoreapp3.1 234ns 0.208ns 0.78ns 0.00577 0 0 424 B
#3109 AllCycleMoreComplexBody net472 187ns 0.402ns 1.56ns 0.0637 0 0 401 B
#3109 AllCycleMoreComplexBody netcoreapp3.1 237ns 0.281ns 1.09ns 0.00542 0 0 400 B
#3109 BodyExtractorSimpleBody net472 264ns 0.193ns 0.721ns 0.0574 0 0 361 B
#3109 BodyExtractorSimpleBody netcoreapp3.1 255ns 0.256ns 0.922ns 0.00363 0 0 272 B
#3109 BodyExtractorMoreComplexBody net472 14.4μs 9.92ns 35.8ns 1.21 0.0215 0 7.62 KB
#3109 BodyExtractorMoreComplexBody netcoreapp3.1 12.4μs 12.6ns 48.9ns 0.0923 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 181μs 208ns 777ns 0.181 0 0 20.33 KB
#3109 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3109 SendRequest netcoreapp3.1 180μs 161ns 580ns 0.18 0 0 20.33 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.52μs 0.725ns 2.71ns 0.126 0.000742 0 794 B
master ExecuteNonQuery netcoreapp3.1 1.32μs 0.33ns 1.24ns 0.0112 0 0 824 B
#3109 ExecuteNonQuery net472 1.52μs 0.634ns 2.46ns 0.126 0.000763 0 794 B
#3109 ExecuteNonQuery netcoreapp3.1 1.26μs 0.277ns 1ns 0.0108 0 0 824 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.23μs 0.532ns 1.92ns 0.159 0 0 1 KB
master CallElasticsearch netcoreapp3.1 1.38μs 0.794ns 2.86ns 0.0131 0 0 984 B
master CallElasticsearchAsync net472 2.4μs 1.08ns 4.19ns 0.181 0 0 1.14 KB
master CallElasticsearchAsync netcoreapp3.1 1.48μs 0.549ns 2.13ns 0.0148 0 0 1.1 KB
#3109 CallElasticsearch net472 2.22μs 0.483ns 1.87ns 0.159 0 0 1 KB
#3109 CallElasticsearch netcoreapp3.1 1.34μs 1.01ns 3.79ns 0.0128 0 0 984 B
#3109 CallElasticsearchAsync net472 2.41μs 1.01ns 3.91ns 0.18 0 0 1.14 KB
#3109 CallElasticsearchAsync netcoreapp3.1 1.48μs 0.762ns 2.95ns 0.0148 0 0 1.1 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.3μs 7.29ns 28.2ns 0.2 0 0 1.26 KB
master ExecuteAsync netcoreapp3.1 1.52μs 4.8ns 18.6ns 0.0166 0 0 1.22 KB
#3109 ExecuteAsync net472 2.33μs 8.58ns 32.1ns 0.2 0 0 1.26 KB
#3109 ExecuteAsync netcoreapp3.1 1.61μs 3.99ns 15.5ns 0.0161 0 0 1.22 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 4.91μs 15.2ns 58.8ns 0.392 0 0 2.48 KB
master SendAsync netcoreapp3.1 3.15μs 3.97ns 14.8ns 0.0319 0 0 2.36 KB
#3109 SendAsync net472 4.86μs 12.1ns 45.2ns 0.393 0 0 2.48 KB
#3109 SendAsync netcoreapp3.1 3.19μs 6.46ns 25ns 0.032 0 0 2.36 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 2.75μs 0.677ns 2.62ns 0.263 0 0 1.66 KB
master EnrichedLog netcoreapp3.1 2.37μs 0.565ns 2.11ns 0.0232 0 0 1.73 KB
#3109 EnrichedLog net472 2.68μs 1.47ns 5.7ns 0.263 0 0 1.66 KB
#3109 EnrichedLog netcoreapp3.1 2.32μs 1.33ns 5.16ns 0.0235 0 0 1.73 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 84.9ns 329ns 0.661 0.22 0 4.5 KB
master EnrichedLog netcoreapp3.1 115μs 270ns 1.05μs 0.0571 0 0 4.38 KB
#3109 EnrichedLog net472 148μs 67.6ns 262ns 0.673 0.224 0 4.5 KB
#3109 EnrichedLog netcoreapp3.1 114μs 208ns 807ns 0.0563 0 0 4.38 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.35μs 18ns 69.9ns 0.545 0.00265 0 3.43 KB
master EnrichedLog netcoreapp3.1 4.09μs 9.07ns 33.9ns 0.0516 0 0 3.8 KB
#3109 EnrichedLog net472 5.33μs 17.2ns 66.6ns 0.545 0.00259 0 3.43 KB
#3109 EnrichedLog netcoreapp3.1 4.14μs 14.4ns 53.7ns 0.0509 0 0 3.8 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.03μs 1.52ns 5.9ns 0.194 0 0 1.22 KB
master SendReceive netcoreapp3.1 1.65μs 0.571ns 2.14ns 0.0165 0 0 1.21 KB
#3109 SendReceive net472 2.02μs 2.25ns 8.42ns 0.193 0 0 1.22 KB
#3109 SendReceive netcoreapp3.1 1.74μs 0.627ns 2.43ns 0.0165 0 0 1.21 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 4.7μs 1.29ns 5ns 0.329 0 0 2.08 KB
master EnrichedLog netcoreapp3.1 4.1μs 0.798ns 2.99ns 0.0225 0 0 1.69 KB
#3109 EnrichedLog net472 4.77μs 2.18ns 8.46ns 0.329 0 0 2.08 KB
#3109 EnrichedLog netcoreapp3.1 4.12μs 1.42ns 5.5ns 0.0231 0 0 1.69 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 821ns 0.876ns 3.39ns 0.104 0 0 658 B
master StartFinishSpan netcoreapp3.1 737ns 0.519ns 1.94ns 0.00882 0 0 648 B
master StartFinishScope net472 1.13μs 0.524ns 2.03ns 0.117 0 0 738 B
master StartFinishScope netcoreapp3.1 902ns 0.846ns 3.27ns 0.0104 0 0 768 B
#3109 StartFinishSpan net472 865ns 0.317ns 1.23ns 0.104 0 0 658 B
#3109 StartFinishSpan netcoreapp3.1 789ns 0.323ns 1.21ns 0.00871 0 0 648 B
#3109 StartFinishScope net472 1.09μs 0.381ns 1.48ns 0.117 0 0 738 B
#3109 StartFinishScope netcoreapp3.1 875ns 0.386ns 1.39ns 0.0104 0 0 768 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.22μs 1.28ns 4.78ns 0.117 0 0 738 B
master RunOnMethodBegin netcoreapp3.1 1.04μs 0.376ns 1.41ns 0.0103 0 0 768 B
#3109 RunOnMethodBegin net472 1.18μs 0.41ns 1.53ns 0.117 0 0 738 B
#3109 RunOnMethodBegin netcoreapp3.1 999ns 0.42ns 1.63ns 0.01 0 0 768 B

@andrewlock

andrewlock commented Aug 22, 2022

Copy link
Copy Markdown
Member Author

Code Coverage Report 📊

✔️ Merging #3109 into master will not change line coverage
✔️ Merging #3109 into master will not change branch coverage
✔️ Merging #3109 into master will not change complexity

master #3109 Change
Lines 16874 / 23281 16923 / 23281
Lines % 72% 73% 0% ✔️
Branches 10067 / 14362 10099 / 14362
Branches % 70% 70% 0% ✔️
Complexity 15458 15458 0 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3109 Change
Lines % 72% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 15458 15458 0 ✔️

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️

View the full reports for further details:

@andrewlock
andrewlock merged commit 4e92b41 into master Aug 23, 2022
@andrewlock
andrewlock deleted the andrew/fix-fnv branch August 23, 2022 13:41
@github-actions github-actions Bot added this to the vNext milestone Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bouwkast bouwkast bouwkast 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:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewlock @bouwkast @kevingosse