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

[PoC] Use Set hash in Distinct #5028

Closed
wants to merge 14 commits into from
Closed

[PoC] Use Set hash in Distinct #5028

wants to merge 14 commits into from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 4, 2024 •
edited
Loading

This presents a proof-of-concept for how to replace the internals of Distinct with a hash of the data held by a Set.

Why do this?

Currently, the Set and Distinct both hold the same data: an interface with that KeyValue array.

The Go runtime has optimized code paths for many built-in types when they are used as a map key. Our current design, using a comparable interface{} pointing to an array, is not as optimized as one of these built-in types.

By replacing the Distinct type with a single hash field (i.e. ~uint64), we can realize performance imporvements without giving up any of the current Set or Distinct behaviors.

$ benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                     │   old.txt   │               new.txt               │
                     │   sec/op    │   sec/op     vs base                │
DistinctMap/Set-8      96.30n ± 4%   22.41n ± 1%  -76.74% (p=0.000 n=10)
DistinctMap/Lookup-8   92.16n ± 3%   20.20n ± 1%  -78.09% (p=0.000 n=10)
geomean                94.21n        21.27n       -77.42%

This is all done by computing the Sets data hash when it is created. This adds minimal overhead to the set creation.

benchstat old_NewSet.txt new_NewSet.txt
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
         │ old_NewSet.txt │           new_NewSet.txt            │
         │     sec/op     │   sec/op     vs base                │
NewSet-8      233.2n ± 4%   266.7n ± 1%  +14.34% (p=0.000 n=10)

         │ old_NewSet.txt │         new_NewSet.txt         │
         │      B/op      │    B/op     vs base            │
NewSet-8       64.00 ± 0%   64.00 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

         │ old_NewSet.txt │         new_NewSet.txt         │
         │   allocs/op    │ allocs/op   vs base            │
NewSet-8       1.000 ± 0%   1.000 ± 0%  ~ (p=1.000 n=10) ¹
¹ all samples are equal

Given the performance costs (NewSet) are generally paid once for many look-ups using that Set (i.e. the metric SDK), and the cost is recouped in the outsized improvement in just one setting/lookup of a map, this change seems like a positive net performance improvement.

Why not do this?

The hashing algorithm used to compute the Set data hash is the fnv-1a hash. This hash was chosen for a few reasons:

The hash/fnv implementation is copied locally and updated to not require any allocations when it is used. This means that it can be checked using the same tests (and even verified by an independent implementation if needed).

This all sounds good, but there is one big issue. This has a low collision rate, but the current implementation does not have any collision rate. Meaning by accepting these changes we will add a very small probability of invalid data setting/look-ups.

@codecov Codecov
Copy link

codecov bot commented Mar 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.11765% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 83.5%. Comparing base (2d968c4) to head (7366740).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5028   +/-   ##
=====================================
  Coverage   83.5%   83.5%           
=====================================
  Files        238     240    +2     
  Lines      15757   15832   +75     
=====================================
+ Hits       13159   13229   +70     
- Misses      2309    2314    +5     
  Partials     289     289
Files Coverage Δ
attribute/internal/fnv/fnv.go 100.0% <100.0%> (ø)
attribute/set.go 78.0% <96.5%> (-0.3%) ⬇️
attribute/hash.go 89.1% <89.1%> (ø)

... and 1 file with indirect coverage changes

@MrAlias MrAlias mentioned this pull request Mar 4, 2024
Merged
@MrAlias MrAlias added the pkg:attribute Related to the attribute package label Mar 5, 2024
@MrAlias MrAlias marked this pull request as ready for review March 5, 2024 21:53
@pellared
Copy link
Member

pellared commented Mar 6, 2024 •
edited
Loading

This all sounds good, but there is one big issue. This has a low collision rate, but the current implementation does not have any collision rate. Meaning by accepting these changes we will add a very small probability of invalid data setting/look-ups.

I find this trade-off is acceptable because:

  • low performance overhead is a very important property for instrumentation; if I understand correctly, this change will increase the performance of the metrics SDK
  • for telemetry, the harm caused by a collision should be acceptable

@MadVikingGod
Copy link
Contributor

Where I would expect to see the real gains are in the metric aggregation benchmarks.

cd sdk/metric/internal/aggregate
go test -bench='.*/.*/.*/100'

I do worry that most users will use WithAttributes vs WithAttributeSet, which could erase these gains. But, I don't think we have a benchmark that measures the difference.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 9, 2024

@MadVikingGod

benchstat old.out new.out
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric/internal/aggregate
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                                                                 │   old.out    │               new.out                │
                                                                 │    sec/op    │    sec/op     vs base                │
ExponentialHistogram/Int64/Cumulative/100/Measure-8                 17.70µ ± 1%   11.12µ ±  1%  -37.17% (p=0.000 n=10)
ExponentialHistogram/Int64/Cumulative/100/ComputeAggregation-8      6.353µ ± 2%   6.564µ ±  1%   +3.31% (p=0.000 n=10)
ExponentialHistogram/Int64/Delta/100/Measure-8                      19.09µ ± 3%   11.23µ ±  1%  -41.18% (p=0.000 n=10)
ExponentialHistogram/Int64/Delta/100/ComputeAggregation-8           6.737µ ± 1%   6.891µ ± 18%   +2.29% (p=0.000 n=10)
ExponentialHistogram/Float64/Cumulative/100/Measure-8               19.21µ ± 4%   11.38µ ±  2%  -40.78% (p=0.000 n=10)
ExponentialHistogram/Float64/Cumulative/100/ComputeAggregation-8    6.365µ ± 1%   6.588µ ± 19%   +3.51% (p=0.000 n=10)
ExponentialHistogram/Float64/Delta/100/Measure-8                    19.71µ ± 3%   11.76µ ±  5%  -40.35% (p=0.000 n=10)
ExponentialHistogram/Float64/Delta/100/ComputeAggregation-8         6.769µ ± 1%   6.911µ ±  1%   +2.09% (p=0.000 n=10)
Histogram/Int64/Cumulative/100/Measure-8                           16.492µ ± 5%   8.696µ ±  6%  -47.27% (p=0.000 n=10)
Histogram/Int64/Cumulative/100/ComputeAggregation-8                 8.978µ ± 0%   9.846µ ± 10%   +9.67% (p=0.000 n=10)
Histogram/Int64/Delta/100/Measure-8                                16.072µ ± 5%   8.351µ ±  2%  -48.04% (p=0.000 n=10)
Histogram/Int64/Delta/100/ComputeAggregation-8                      4.525µ ± 0%   4.550µ ±  2%   +0.56% (p=0.009 n=10)
Histogram/Float64/Cumulative/100/Measure-8                         16.654µ ± 3%   8.704µ ±  4%  -47.74% (p=0.000 n=10)
Histogram/Float64/Cumulative/100/ComputeAggregation-8               9.050µ ± 1%   9.308µ ± 15%   +2.85% (p=0.000 n=10)
Histogram/Float64/Delta/100/Measure-8                              16.417µ ± 6%   8.605µ ±  3%  -47.58% (p=0.000 n=10)
Histogram/Float64/Delta/100/ComputeAggregation-8                    4.547µ ± 1%   4.459µ ±  1%   -1.92% (p=0.000 n=10)
Sum/Int64/Cumulative/100/Measure-8                                 25.951µ ± 3%   9.133µ ±  5%  -64.80% (p=0.000 n=10)
Sum/Int64/Cumulative/100/ComputeAggregation-8                       2.458µ ± 1%   2.468µ ± 10%        ~ (p=0.171 n=10)
Sum/Int64/Delta/100/Measure-8                                      26.016µ ± 3%   9.066µ ±  4%  -65.15% (p=0.000 n=10)
Sum/Int64/Delta/100/ComputeAggregation-8                            2.758µ ± 1%   2.783µ ±  0%   +0.92% (p=0.001 n=10)
Sum/Float64/Cumulative/100/Measure-8                               25.480µ ± 3%   9.607µ ±  5%  -62.30% (p=0.000 n=10)
Sum/Float64/Cumulative/100/ComputeAggregation-8                     2.459µ ± 0%   2.466µ ±  1%        ~ (p=0.305 n=10)
Sum/Float64/Delta/100/Measure-8                                    25.425µ ± 4%   9.368µ ±  6%  -63.16% (p=0.000 n=10)
Sum/Float64/Delta/100/ComputeAggregation-8                          2.751µ ± 0%   2.794µ ±  5%   +1.58% (p=0.000 n=10)
geomean                                                             9.770µ        6.871µ        -29.68%

                                                                 │    old.out     │                new.out                │
                                                                 │      B/op      │     B/op      vs base                 │
ExponentialHistogram/Int64/Cumulative/100/Measure-8                  0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Cumulative/100/ComputeAggregation-8       32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Delta/100/Measure-8                       0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Delta/100/ComputeAggregation-8            32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Cumulative/100/Measure-8                0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Cumulative/100/ComputeAggregation-8     32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Delta/100/Measure-8                     0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Delta/100/ComputeAggregation-8          32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/100/Measure-8                             0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/100/ComputeAggregation-8                2.391Ki ± 0%     2.391Ki ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/100/Measure-8                                  0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/100/ComputeAggregation-8                       48.00 ± 0%       48.00 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/100/Measure-8                           0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/100/ComputeAggregation-8              2.391Ki ± 0%     2.391Ki ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/100/Measure-8                                0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/100/ComputeAggregation-8                     48.00 ± 0%       48.00 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Cumulative/100/Measure-8                                   0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Cumulative/100/ComputeAggregation-8                        32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Delta/100/Measure-8                                        0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Delta/100/ComputeAggregation-8                             32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Cumulative/100/Measure-8                                 0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Cumulative/100/ComputeAggregation-8                      32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Delta/100/Measure-8                                      0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Delta/100/ComputeAggregation-8                           32.00 ± 0%       32.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                                         ²                 +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                                                 │   old.out    │               new.out               │
                                                                 │  allocs/op   │ allocs/op   vs base                 │
ExponentialHistogram/Int64/Cumulative/100/Measure-8                0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Cumulative/100/ComputeAggregation-8     1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Delta/100/Measure-8                     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Int64/Delta/100/ComputeAggregation-8          1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Cumulative/100/Measure-8              0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Cumulative/100/ComputeAggregation-8   1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Delta/100/Measure-8                   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ExponentialHistogram/Float64/Delta/100/ComputeAggregation-8        1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/100/Measure-8                           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Cumulative/100/ComputeAggregation-8                102.0 ± 0%     102.0 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/100/Measure-8                                0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Int64/Delta/100/ComputeAggregation-8                     2.000 ± 0%     2.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/100/Measure-8                         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Cumulative/100/ComputeAggregation-8              102.0 ± 0%     102.0 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/100/Measure-8                              0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Histogram/Float64/Delta/100/ComputeAggregation-8                   2.000 ± 0%     2.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Cumulative/100/Measure-8                                 0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Cumulative/100/ComputeAggregation-8                      1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Delta/100/Measure-8                                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Int64/Delta/100/ComputeAggregation-8                           1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Cumulative/100/Measure-8                               0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Cumulative/100/ComputeAggregation-8                    1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Delta/100/Measure-8                                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Sum/Float64/Delta/100/ComputeAggregation-8                         1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                                       ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 14, 2024

cc @dashpole

@MrAlias MrAlias closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@pellared pellared Awaiting requested review from pellared pellared is a code owner

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@dmathieu dmathieu Awaiting requested review from dmathieu dmathieu is a code owner

Assignees
No one assigned
Labels
pkg:attribute Related to the attribute package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MrAlias @pellared @MadVikingGod