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

Add an interned TagKey class.#124

Merged
isturdy merged 1 commit intocensus-instrumentation:masterfrom
isturdy:interned-tag-keys-nodp
Mar 27, 2018
Merged

Add an interned TagKey class.#124
isturdy merged 1 commit intocensus-instrumentation:masterfrom
isturdy:interned-tag-keys-nodp

Conversation

@isturdy
Copy link
Copy Markdown
Contributor

@isturdy isturdy commented Mar 23, 2018 •
edited
Loading

Implements #110.

@isturdy isturdy requested review from bogdandrutu and g-easy March 23, 2018 18:38
@g-easy
Copy link
Copy Markdown
Contributor

g-easy commented Mar 27, 2018

Please update commit description with text from issue #110 to explain why this is being done.

Is it worth including benchmark results here?

g-easy
g-easy approved these changes Mar 27, 2018
View reviewed changes
Comment thread opencensus/stats/internal/tag_key.cc Outdated

TagKey Register(absl::string_view name);

// Return string_view rather than a const ref so that returned values will not
Copy link
Copy Markdown
Contributor

@g-easy g-easy Mar 27, 2018

Choose a reason for hiding this comment

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

But it returns a constref?

Copy link
Copy Markdown
Contributor Author

@isturdy isturdy Mar 27, 2018

Choose a reason for hiding this comment

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

Removed comment. I went back on that because most of the uses required a std::string and the string_view interface thus caused a lot of copies. This should not be a problem in normal use where tag keys are initialized during startup.

Comment thread opencensus/stats/internal/tag_key_test.cc Outdated

#include "opencensus/stats/tag_key.h"

#include "gmock/gmock.h"
Copy link
Copy Markdown
Contributor

@g-easy g-easy Mar 27, 2018

Choose a reason for hiding this comment

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

Don't need mock.

Copy link
Copy Markdown
Contributor Author

@isturdy isturdy Mar 27, 2018

Choose a reason for hiding this comment

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

Done.

@isturdy isturdy force-pushed the interned-tag-keys-nodp branch from 44dbe51 to df00b27 Compare March 27, 2018 17:53
@isturdy
Copy link
Copy Markdown
Contributor Author

isturdy commented Mar 27, 2018

Benchmark results:

Benchmark                                       Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------
BM_RecordBatched_mean                        -0.0639         -0.0615          3646          3413          3634          3410
BM_DeltaProducerRecordBatched_mean           -0.1792         -0.1770           387           318           386           318

Add an interned TagKey class.
d38654b 
Previously, tag keys were strings, and use string semantics. This has
performance implications--we need to copy/hash/check equality
frequently during aggregation. Interning tag keys makes those integer
operations, substantially improving performance.
@isturdy isturdy force-pushed the interned-tag-keys-nodp branch from f4932fc to d38654b Compare March 27, 2018 17:59
@isturdy isturdy merged commit a73894c into census-instrumentation:master Mar 27, 2018
@isturdy isturdy deleted the interned-tag-keys-nodp branch March 27, 2018 18:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu bogdandrutu is a code owner

1 more reviewer

@g-easy g-easy g-easy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@isturdy @g-easy