Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Convert Attribute to a struct #581

Merged

Conversation

semistrict
Copy link
Contributor

This avoid about 30% of allocations in benchmarks with 3 or more
attributes.

name                     old time/op    new time/op    delta
StartEndSpan-8              522ns ± 0%     502ns ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_3-8    1.22µs ± 0%    1.08µs ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_6-8    1.66µs ± 0%    1.29µs ± 0%   ~     (p=1.000 n=1+1)
TraceID_DotString-8         191ns ± 0%     185ns ± 0%   ~     (p=1.000 n=1+1)
SpanID_DotString-8          172ns ± 0%     174ns ± 0%   ~     (p=1.000 n=1+1)

name                     old alloc/op   new alloc/op   delta
StartEndSpan-8               576B ± 0%      576B ± 0%   ~     (all equal)
SpanWithAnnotations_3-8    1.37kB ± 0%    1.27kB ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_6-8    1.49kB ± 0%    1.30kB ± 0%   ~     (p=1.000 n=1+1)
TraceID_DotString-8         80.0B ± 0%     80.0B ± 0%   ~     (all equal)
SpanID_DotString-8          56.0B ± 0%     56.0B ± 0%   ~     (all equal)

name                     old allocs/op  new allocs/op  delta
StartEndSpan-8               4.00 ± 0%      4.00 ± 0%   ~     (all equal)
SpanWithAnnotations_3-8      13.0 ± 0%      10.0 ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_6-8      18.0 ± 0%      12.0 ± 0%   ~     (p=1.000 n=1+1)
TraceID_DotString-8          3.00 ± 0%      3.00 ± 0%   ~     (all equal)
SpanID_DotString-8           3.00 ± 0%      3.00 ± 0%   ~     (all equal)

@semistrict semistrict force-pushed the attribute-struct branch 2 times, most recently from f617bda to 48c6432 Compare March 14, 2018 04:04
This avoids about 30% of allocations in benchmarks with 3 or more
attributes and sampling enabled.

```
name                     old time/op    new time/op    delta
StartEndSpan-8              522ns ± 0%     502ns ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_3-8    1.22µs ± 0%    1.08µs ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_6-8    1.66µs ± 0%    1.29µs ± 0%   ~     (p=1.000 n=1+1)
TraceID_DotString-8         191ns ± 0%     185ns ± 0%   ~     (p=1.000 n=1+1)
SpanID_DotString-8          172ns ± 0%     174ns ± 0%   ~     (p=1.000 n=1+1)

name                     old alloc/op   new alloc/op   delta
StartEndSpan-8               576B ± 0%      576B ± 0%   ~     (all equal)
SpanWithAnnotations_3-8    1.37kB ± 0%    1.27kB ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_6-8    1.49kB ± 0%    1.30kB ± 0%   ~     (p=1.000 n=1+1)
TraceID_DotString-8         80.0B ± 0%     80.0B ± 0%   ~     (all equal)
SpanID_DotString-8          56.0B ± 0%     56.0B ± 0%   ~     (all equal)

name                     old allocs/op  new allocs/op  delta
StartEndSpan-8               4.00 ± 0%      4.00 ± 0%   ~     (all equal)
SpanWithAnnotations_3-8      13.0 ± 0%      10.0 ± 0%   ~     (p=1.000 n=1+1)
SpanWithAnnotations_6-8      18.0 ± 0%      12.0 ± 0%   ~     (p=1.000 n=1+1)
TraceID_DotString-8          3.00 ± 0%      3.00 ± 0%   ~     (all equal)
SpanID_DotString-8           3.00 ± 0%      3.00 ± 0%   ~     (all equal)
```
@semistrict
Copy link
Contributor Author

Not sure why the delta column displays just "~" but you can see that the numbers did change. This test compares the numbers after making everything sampled (before, we didn't benchmark the sampled case, I also added sampled/not-sampled benchmarks).

@semistrict semistrict requested a review from rakyll March 14, 2018 04:10
@odeke-em
Copy link
Member

Not sure why the delta column displays just "~" but you can see that the numbers did change

If you didn't already, could you please run the benchmarks say 10 times or so e.g

go test -run=^$ -bench=. -count=10

@semistrict
Copy link
Contributor Author

Unfortunately it isn't easy to reproduce the above table with "count=10" because I changed the benchmarks themselves in this PR. In any case, the performance gain is not the only reason for this change - it is more idiomatic (returning structs rather than interfaces) and it's a net code reduction (except for more test code).

@semistrict semistrict merged commit 821173a into census-instrumentation:master Mar 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants