-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: main
Are you sure you want to change the base?
Conversation
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) ```
|
Could you use benchstat to make the benchmark more comparable ? You also need to sign the CLA |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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 🚀 New features to boost your workflow:
|
The output I posted did come from benchstat, yes. Is there some other formatting mode you typically use with it?
Yup, I'm waiting on my company's approvers. Will the CLA status here update automatically when they approve me? |
Urgh you're right, it's my friday evening mistake. |
Replace a manual loop in
computeDistinctReflect
with a direct call toreflect.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.