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.

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

attribute: faster NewSet, NewSetWithFiltered #6422

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akehlenbeck
Copy link

@akehlenbeck akehlenbeck commented Mar 7, 2025

Replace a manual loop in computeDistinctReflect with a direct call to reflect.Value.Convert(). The resulting code is both simpler and faster.

The "len" parameter in the following benchmarks indicates the number of attributes in the set. Sets with ten or fewer attributes use a non-reflection-based codepath that isn't modified by this change.

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: AMD EPYC 7B13

name             old time/op    new time/op    delta
NewSet/len=1-8     42.8ns ± 1%    42.2ns ± 1%   -1.23%  (p=0.016 n=5+5)
NewSet/len=2-8     71.4ns ± 1%    71.5ns ± 1%     ~     (p=1.000 n=5+5)
NewSet/len=3-8      106ns ± 1%     106ns ± 2%     ~     (p=0.881 n=5+5)
NewSet/len=4-8      127ns ± 1%     129ns ± 2%   +1.49%  (p=0.016 n=5+5)
NewSet/len=5-8      161ns ± 2%     157ns ± 2%   -1.94%  (p=0.024 n=5+5)
NewSet/len=6-8      187ns ± 1%     188ns ± 1%     ~     (p=0.460 n=5+5)
NewSet/len=7-8      212ns ± 3%     212ns ± 1%     ~     (p=0.952 n=5+5)
NewSet/len=8-8      237ns ± 2%     237ns ± 2%     ~     (p=1.000 n=5+5)
NewSet/len=9-8      265ns ± 1%     265ns ± 1%     ~     (p=0.841 n=5+5)
NewSet/len=10-8     291ns ± 1%     289ns ± 1%     ~     (p=0.143 n=5+5)
NewSet/len=11-8     666ns ± 4%     384ns ± 1%  -42.31%  (p=0.008 n=5+5)
NewSet/len=12-8     711ns ± 1%     414ns ± 1%  -41.73%  (p=0.008 n=5+5)
NewSet/len=13-8     752ns ± 1%     437ns ± 1%  -41.90%  (p=0.008 n=5+5)
NewSet/len=14-8     818ns ± 2%     464ns ± 1%  -43.29%  (p=0.008 n=5+5)
NewSet/len=15-8     866ns ± 1%     483ns ± 0%  -44.21%  (p=0.008 n=5+5)
NewSet/len=16-8     914ns ± 1%     522ns ± 3%  -42.89%  (p=0.008 n=5+5)
NewSet/len=17-8     980ns ± 2%     536ns ± 1%  -45.34%  (p=0.008 n=5+5)
NewSet/len=18-8    1.06µs ± 2%    0.58µs ± 1%  -45.54%  (p=0.008 n=5+5)
NewSet/len=19-8    1.07µs ± 1%    0.59µs ± 1%  -44.49%  (p=0.008 n=5+5)
NewSet/len=20-8    1.11µs ± 1%    0.63µs ± 1%  -43.33%  (p=0.008 n=5+5)

name             old alloc/op   new alloc/op   delta
NewSet/len=1-8      64.0B ± 0%     64.0B ± 0%     ~     (all equal)
NewSet/len=2-8       128B ± 0%      128B ± 0%     ~     (all equal)
NewSet/len=3-8       192B ± 0%      192B ± 0%     ~     (all equal)
NewSet/len=4-8       256B ± 0%      256B ± 0%     ~     (all equal)
NewSet/len=5-8       320B ± 0%      320B ± 0%     ~     (all equal)
NewSet/len=6-8       384B ± 0%      384B ± 0%     ~     (all equal)
NewSet/len=7-8       448B ± 0%      448B ± 0%     ~     (all equal)
NewSet/len=8-8       512B ± 0%      512B ± 0%     ~     (all equal)
NewSet/len=9-8       640B ± 0%      640B ± 0%     ~     (all equal)
NewSet/len=10-8      704B ± 0%      704B ± 0%     ~     (all equal)
NewSet/len=11-8    1.54kB ± 0%    0.79kB ± 0%  -48.44%  (p=0.008 n=5+5)
NewSet/len=12-8    1.79kB ± 0%    0.92kB ± 0%  -48.66%  (p=0.008 n=5+5)
NewSet/len=13-8    1.79kB ± 0%    0.92kB ± 0%  -48.66%  (p=0.008 n=5+5)
NewSet/len=14-8    2.05kB ± 0%    1.05kB ± 0%  -48.83%  (p=0.008 n=5+5)
NewSet/len=15-8    2.05kB ± 0%    1.05kB ± 0%  -48.83%  (p=0.008 n=5+5)
NewSet/len=16-8    2.30kB ± 0%    1.18kB ± 0%  -48.96%  (p=0.008 n=5+5)
NewSet/len=17-8    2.30kB ± 0%    1.18kB ± 0%  -48.96%  (p=0.008 n=5+5)
NewSet/len=18-8    2.56kB ± 0%    1.30kB ± 0%  -49.06%  (p=0.008 n=5+5)
NewSet/len=19-8    2.56kB ± 0%    1.30kB ± 0%  -49.06%  (p=0.008 n=5+5)
NewSet/len=20-8    2.82kB ± 0%    1.43kB ± 0%  -49.15%  (p=0.008 n=5+5)

Replace a manual loop in `computeDistinctReflect` with a direct call
to `reflect.Value.Convert()`. The resulting code is both simpler and
faster.

The "len" parameter in the following benchmarks indicates the number
of attributes in the set. Sets with ten or fewer attribute use a
non-reflection-based codepath that isn't modified by this change.

```
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/attribute
cpu: AMD EPYC 7B13

name             old time/op    new time/op    delta
NewSet/len=1-8     42.8ns ± 1%    42.2ns ± 1%   -1.23%  (p=0.016 n=5+5)
NewSet/len=2-8     71.4ns ± 1%    71.5ns ± 1%     ~     (p=1.000 n=5+5)
NewSet/len=3-8      106ns ± 1%     106ns ± 2%     ~     (p=0.881 n=5+5)
NewSet/len=4-8      127ns ± 1%     129ns ± 2%   +1.49%  (p=0.016 n=5+5)
NewSet/len=5-8      161ns ± 2%     157ns ± 2%   -1.94%  (p=0.024 n=5+5)
NewSet/len=6-8      187ns ± 1%     188ns ± 1%     ~     (p=0.460 n=5+5)
NewSet/len=7-8      212ns ± 3%     212ns ± 1%     ~     (p=0.952 n=5+5)
NewSet/len=8-8      237ns ± 2%     237ns ± 2%     ~     (p=1.000 n=5+5)
NewSet/len=9-8      265ns ± 1%     265ns ± 1%     ~     (p=0.841 n=5+5)
NewSet/len=10-8     291ns ± 1%     289ns ± 1%     ~     (p=0.143 n=5+5)
NewSet/len=11-8     666ns ± 4%     384ns ± 1%  -42.31%  (p=0.008 n=5+5)
NewSet/len=12-8     711ns ± 1%     414ns ± 1%  -41.73%  (p=0.008 n=5+5)
NewSet/len=13-8     752ns ± 1%     437ns ± 1%  -41.90%  (p=0.008 n=5+5)
NewSet/len=14-8     818ns ± 2%     464ns ± 1%  -43.29%  (p=0.008 n=5+5)
NewSet/len=15-8     866ns ± 1%     483ns ± 0%  -44.21%  (p=0.008 n=5+5)
NewSet/len=16-8     914ns ± 1%     522ns ± 3%  -42.89%  (p=0.008 n=5+5)
NewSet/len=17-8     980ns ± 2%     536ns ± 1%  -45.34%  (p=0.008 n=5+5)
NewSet/len=18-8    1.06µs ± 2%    0.58µs ± 1%  -45.54%  (p=0.008 n=5+5)
NewSet/len=19-8    1.07µs ± 1%    0.59µs ± 1%  -44.49%  (p=0.008 n=5+5)
NewSet/len=20-8    1.11µs ± 1%    0.63µs ± 1%  -43.33%  (p=0.008 n=5+5)

name             old alloc/op   new alloc/op   delta
NewSet/len=1-8      64.0B ± 0%     64.0B ± 0%     ~     (all equal)
NewSet/len=2-8       128B ± 0%      128B ± 0%     ~     (all equal)
NewSet/len=3-8       192B ± 0%      192B ± 0%     ~     (all equal)
NewSet/len=4-8       256B ± 0%      256B ± 0%     ~     (all equal)
NewSet/len=5-8       320B ± 0%      320B ± 0%     ~     (all equal)
NewSet/len=6-8       384B ± 0%      384B ± 0%     ~     (all equal)
NewSet/len=7-8       448B ± 0%      448B ± 0%     ~     (all equal)
NewSet/len=8-8       512B ± 0%      512B ± 0%     ~     (all equal)
NewSet/len=9-8       640B ± 0%      640B ± 0%     ~     (all equal)
NewSet/len=10-8      704B ± 0%      704B ± 0%     ~     (all equal)
NewSet/len=11-8    1.54kB ± 0%    0.79kB ± 0%  -48.44%  (p=0.008 n=5+5)
NewSet/len=12-8    1.79kB ± 0%    0.92kB ± 0%  -48.66%  (p=0.008 n=5+5)
NewSet/len=13-8    1.79kB ± 0%    0.92kB ± 0%  -48.66%  (p=0.008 n=5+5)
NewSet/len=14-8    2.05kB ± 0%    1.05kB ± 0%  -48.83%  (p=0.008 n=5+5)
NewSet/len=15-8    2.05kB ± 0%    1.05kB ± 0%  -48.83%  (p=0.008 n=5+5)
NewSet/len=16-8    2.30kB ± 0%    1.18kB ± 0%  -48.96%  (p=0.008 n=5+5)
NewSet/len=17-8    2.30kB ± 0%    1.18kB ± 0%  -48.96%  (p=0.008 n=5+5)
NewSet/len=18-8    2.56kB ± 0%    1.30kB ± 0%  -49.06%  (p=0.008 n=5+5)
NewSet/len=19-8    2.56kB ± 0%    1.30kB ± 0%  -49.06%  (p=0.008 n=5+5)
NewSet/len=20-8    2.82kB ± 0%    1.43kB ± 0%  -49.15%  (p=0.008 n=5+5)
```
Copy link

linux-foundation-easycla bot commented Mar 7, 2025

CLA Not Signed

@dmathieu
Copy link
Member

dmathieu commented Mar 7, 2025

Could you use benchstat to make the benchmark more comparable ?

You also need to sign the CLA

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (838643a) to head (9cbe3ad).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6422   +/-   ##
=====================================
  Coverage   81.8%   81.8%           
=====================================
  Files        283     283           
  Lines      24899   24895    -4     
=====================================
- Hits       20379   20377    -2     
+ Misses      4117    4116    -1     
+ Partials     403     402    -1     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MrAlias MrAlias added the blocked:CLA Waiting on CLA to be signed before progress can be made label Mar 7, 2025
@akehlenbeck
Copy link
Author

akehlenbeck commented Mar 7, 2025

Could you use benchstat to make the benchmark more comparable ?

The output I posted did come from benchstat, yes. Is there some other formatting mode you typically use with it?

You also need to sign the CLA

Yup, I'm waiting on my company's approvers. Will the CLA status here update automatically when they approve me?

@dmathieu
Copy link
Member

dmathieu commented Mar 7, 2025

Urgh you're right, it's my friday evening mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:CLA Waiting on CLA to be signed before progress can be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants