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

Optimize Record() to avoid extra allocations #1267

Conversation

howardjohn
Copy link
Contributor

Currently, Record() re-uses code with RecordWithOptions. This always creates
allocations for createRecordOption, which is not needed in this case - we only
have measurements and not generic options.

With a little code duplication, we can reduce these allocations.

name                    old time/op    new time/op    delta
Record0-6                 92.2ns ± 9%     1.7ns ± 4%   -98.11%  (p=0.008 n=5+5)
Record1-6                  665ns ± 5%     634ns ± 6%    -4.57%  (p=0.095 n=5+5)
Record8-6                 1.24µs ± 5%    1.21µs ± 5%    -2.18%  (p=0.206 n=5+5)
Record8_WithRecorder-6     796ns ± 5%     777ns ± 5%    -2.45%  (p=0.222 n=5+5)
Record8_Parallel-6        1.21µs ± 2%    1.26µs ±24%      ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 4%    1.23µs ± 2%      ~     (p=0.968 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  80.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
Record1-6                   200B ± 0%      120B ± 0%   -40.00%  (p=0.008 n=5+5)
Record8-6                   424B ± 0%      344B ± 0%   -18.87%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%      ~     (all equal)
Record8_Parallel-6          424B ± 0%      344B ± 0%   -18.87%  (p=0.008 n=5+5)
Record8_8Tags-6             424B ± 0%      344B ± 0%   -18.87%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
Record1-6                   4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
Record8-6                   4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%      ~     (all equal)
Record8_Parallel-6          4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
Record8_8Tags-6             4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)

Note to reviewers: This PR has two commits. The first is broken out into #1266; likely that should be merged first
For #1265

@howardjohn howardjohn requested review from rghetia and a team as code owners September 14, 2021 17:09
@google-cla google-cla bot added the cla: yes label Sep 14, 2021
howardjohn added a commit to howardjohn/opencensus-go that referenced this pull request Sep 14, 2021
This is built upon
census-instrumentation#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to census-instrumentation#1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```
@howardjohn
Copy link
Contributor Author

#1270 fixes the build failure

Currently, the metric we record is not registered. This hits the
fast-path code of not actually recording the metric, so we miss out on
detecting any performance to that main code path.

This registers the metrics so we actually trigger `record`.
Currently, `Record()` re-uses code with `RecordWithOptions`. This always creates
allocations for createRecordOption, which is not needed in this case - we only
have measurements and not generic options.

With a little code duplication, we can reduce these allocations.

```
name                    old time/op    new time/op    delta
Record0-6                 92.2ns ± 9%     1.7ns ± 4%   -98.11%  (p=0.008 n=5+5)
Record1-6                  665ns ± 5%     634ns ± 6%    -4.57%  (p=0.095 n=5+5)
Record8-6                 1.24µs ± 5%    1.21µs ± 5%    -2.18%  (p=0.206 n=5+5)
Record8_WithRecorder-6     796ns ± 5%     777ns ± 5%    -2.45%  (p=0.222 n=5+5)
Record8_Parallel-6        1.21µs ± 2%    1.26µs ±24%      ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 4%    1.23µs ± 2%      ~     (p=0.968 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  80.0B ± 0%      0.0B       -100.00%  (p=0.008 n=5+5)
Record1-6                   200B ± 0%      120B ± 0%   -40.00%  (p=0.008 n=5+5)
Record8-6                   424B ± 0%      344B ± 0%   -18.87%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%      ~     (all equal)
Record8_Parallel-6          424B ± 0%      344B ± 0%   -18.87%  (p=0.008 n=5+5)
Record8_8Tags-6             424B ± 0%      344B ± 0%   -18.87%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
Record1-6                   4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
Record8-6                   4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%      ~     (all equal)
Record8_Parallel-6          4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
Record8_8Tags-6             4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.008 n=5+5)
```
@dashpole dashpole merged commit a55fb71 into census-instrumentation:master Sep 14, 2021
howardjohn added a commit to howardjohn/opencensus-go that referenced this pull request Sep 14, 2021
This is built upon
census-instrumentation#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to census-instrumentation#1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```
howardjohn added a commit to howardjohn/opencensus-go that referenced this pull request Sep 14, 2021
This is built upon
census-instrumentation#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to census-instrumentation#1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```
howardjohn added a commit to howardjohn/opencensus-go that referenced this pull request Sep 14, 2021
This is built upon
census-instrumentation#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to census-instrumentation#1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```
howardjohn added a commit to howardjohn/opencensus-go that referenced this pull request Sep 14, 2021
This is built upon
census-instrumentation#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to census-instrumentation#1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```
howardjohn added a commit to howardjohn/opencensus-go that referenced this pull request Sep 14, 2021
This is built upon
census-instrumentation#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to census-instrumentation#1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```
dashpole pushed a commit that referenced this pull request Oct 21, 2021
* Remove `convTslice` calls in `Record()`

This is built upon
#1267; that
one should likely merge first. I split this out as it has a small public
API change (to work around circular imports) to avoid issues on the
first PR.

Benchmark relative to #1267:
```
me                    old time/op    new time/op    delta
Record0-6                 1.74ns ± 4%    1.79ns ± 2%   +2.85%  (p=0.238 n=5+5)
Record1-6                  634ns ± 6%     542ns ± 9%  -14.55%  (p=0.008 n=5+5)
Record8-6                 1.21µs ± 5%    1.23µs ± 2%   +1.97%  (p=0.254 n=5+5)
Record8_WithRecorder-6     777ns ± 5%     792ns ± 5%   +1.97%  (p=0.421 n=5+5)
Record8_Parallel-6        1.26µs ±24%    1.22µs ± 2%     ~     (p=0.690 n=5+5)
Record8_8Tags-6           1.23µs ± 2%    1.25µs ± 3%     ~     (p=0.651 n=5+5)

name                    old alloc/op   new alloc/op   delta
Record0-6                  0.00B          0.00B          ~     (all equal)
Record1-6                   120B ± 0%       96B ± 0%  -20.00%  (p=0.008 n=5+5)
Record8-6                   344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_WithRecorder-6      424B ± 0%      424B ± 0%     ~     (all equal)
Record8_Parallel-6          344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)
Record8_8Tags-6             344B ± 0%      320B ± 0%   -6.98%  (p=0.008 n=5+5)

name                    old allocs/op  new allocs/op  delta
Record0-6                   0.00           0.00          ~     (all equal)
Record1-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8-6                   3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_WithRecorder-6      4.00 ± 0%      4.00 ± 0%     ~     (all equal)
Record8_Parallel-6          3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
Record8_8Tags-6             3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.008 n=5+5)
```

* Refactor to avoid leaking into public API
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants