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

Optimize open-census usage #416

Merged
merged 1 commit into from Sep 14, 2021

Conversation

howardjohn
Copy link
Member

See census-instrumentation/opencensus-go#1265

Benchmark:

name       old time/op    new time/op    delta
Counter-6     713ns ± 8%     680ns ± 3%   -4.63%  (p=0.006 n=10+9)

name       old alloc/op   new alloc/op   delta
Counter-6      205B ± 0%      173B ± 0%  -15.63%  (p=0.000 n=10+10)

name       old allocs/op  new allocs/op  delta
Counter-6      6.00 ± 0%      5.00 ± 0%  -16.67%  (p=0.000 n=10+10)

This is not that much faster, but once my OC PRs merge we can take advantage of more optimizations this way, bringing us down to:

name       old time/op    new time/op    delta
Counter-6     713ns ± 8%     618ns ± 6%  -13.35%  (p=0.000 n=10+9)

name       old alloc/op   new alloc/op   delta
Counter-6      205B ± 0%       68B ± 1%  -66.59%  (p=0.000 n=10+10)

name       old allocs/op  new allocs/op  delta
Counter-6      6.00 ± 0%      3.00 ± 0%  -50.00%  (p=0.000 n=10+10)
``

@howardjohn howardjohn requested a review from a team as a code owner September 14, 2021 18:10
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 14, 2021
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 14, 2021
Copy link
Contributor

@douglas-reid douglas-reid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me. Thanks for tackling!

tags []tag.Mutator
// ctx is a precomputed context holding tags, as an optimization
ctx context.Context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This technically violates the explicit documentation for context.Context:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

But I'm not sure that matters for this use case. This is a vestige of the OC library construction, and is not holding request-scoped information or any sort of deadlines, etc.

@istio-testing istio-testing merged commit da9399e into istio:master Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
4 participants