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

Adding noop metrics benchmark #1607

Merged
merged 6 commits into from
Mar 11, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Mar 8, 2024

Fixes #

Changes

Adding benchmark for Counter and Vector of KeyValue creation. Rest of the instruments should have benchmarks same as the
counter.

Results:
NoopCounter_NoAttributes: 1.5685 ns
NoopCounter_AddWithInlineStaticAttributes: 15.452 ns
NoopCounter_AddWithStaticArray: 15.354 ns
NoopCounter_AddWithDynamicAttributes: 25.692 ns
CreateVector_KeyValue: 24.515 ns
CreateDynamicVector_StringPair: 117.25 ps

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team March 8, 2024 00:48
@lalitb lalitb marked this pull request as draft March 8, 2024 00:49
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.0%. Comparing base (4aa4827) to head (42731f5).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1607   +/-   ##
=====================================
  Coverage   67.0%   67.0%           
=====================================
  Files        138     138           
  Lines      19515   19515           
=====================================
  Hits       13084   13084           
  Misses      6431    6431           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lalitb lalitb marked this pull request as ready for review March 8, 2024 02:32
Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM

@TommyCpp TommyCpp merged commit e816cb9 into open-telemetry:main Mar 11, 2024
14 of 15 checks passed
@cijothomas
Copy link
Member

CreateVector_KeyValue: 24.515 ns
CreateDynamicVector_StringPair: 117.25 ps

This is....interesting. Wondering if it is due to copy or clone or something else inside Key,Value,KeyValue in opentelemetry! Should steal/learn/copy from how tracing handles similar needs.

@lalitb
Copy link
Member Author

lalitb commented Mar 11, 2024

CreateDynamicVector_StringPair: 117.25 ps

This is....interesting. Wondering if it is due to copy or clone or something else inside Key,Value,KeyValue in opentelemetry! Should steal/learn/copy from how tracing handles similar needs.

Yes, 117.25 ps is interesting, it appears to be shorter than the instruction cycle. This might not reflect the actual cost, - the compiler could be doing some optimization in terms of creating the tuples for the vector at compile time. Or worst, the vector though created is not being used, so compiler might eliminate this code altogether. Let me check if it is possible to turn off compiler optimization in the benchmark tests and get true cost.

@lalitb
Copy link
Member Author

lalitb commented Mar 11, 2024

while this got merged, I just noticed that the PR has triggered the CI error in main branch:

error: package `half v2.4.0` cannot be built because it requires rustc 1.70 or newer, while the currently active rustc version is 1.65.0

The only change was the inclusion of criterion crate in opentelemetry Cargo.toml. Will check it further, but in case there are suggestions how to fix it :)

@lalitb lalitb mentioned this pull request Mar 11, 2024
4 tasks
@cijothomas
Copy link
Member

CreateDynamicVector_StringPair: 117.25 ps

This is....interesting. Wondering if it is due to copy or clone or something else inside Key,Value,KeyValue in opentelemetry! Should steal/learn/copy from how tracing handles similar needs.

Yes, 117.25 ps is interesting, it appears to be shorter than the instruction cycle. This might not reflect the actual cost, - the compiler could be doing some optimization in terms of creating the tuples for the vector at compile time. Or worst, the vector though created is not being used, so compiler might eliminate this code altogether. Let me check if it is possible to turn off compiler optimization in the benchmark tests and get true cost.

Yes please! We need to make sure compiler treat this as a semi-black-box.
But I was more curious about the 24ns it took when only "no-op" is in play!

@lalitb
Copy link
Member Author

lalitb commented Mar 11, 2024

But I was more curious about the 24ns it took when only "no-op" is in play!

There should not be the any internal copy happening for Key and Value in this specific test. As we are passing string literals for Key and Value, they should just get type-converted to OtelString::Static. The run time memory overhead should be for the vector allocation, which stores the KeyValue objects, which internally refer to the static string liternals.

@cijothomas
Copy link
Member

But I was more curious about the 24ns it took when only "no-op" is in play!

There should not be the any internal copy happening for Key and Value in this specific test. As we are passing string literals for Key and Value, they should just get type-converted to OtelString::Static. The run time memory overhead should be for the vector allocation, which stores the KeyValue objects, which internally refer to the static string liternals.

Yes I think so too. What is(was) misleading is that, the CreateDynamicVector_StringPair shows that allocating vec is cheap. But as you said, its most likely due to compiler optimizations, and not doing what we think it is doing!!

@lalitb lalitb mentioned this pull request Mar 11, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants