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

Don't copy bytes in the exemplar.Collect loop #6145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

grcevski
Copy link

@grcevski grcevski commented Jan 9, 2025

While reviewing code for a project I'm working on I noticed that the Collect loop is copying 136 bytes on each iteration of the loop. I'm submitting a small PR to change the loop to use a measurement pointer instead of a value.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.2%. Comparing base (14b874e) to head (76da265).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #6145   +/-   ##
=====================================
  Coverage   82.2%   82.2%           
=====================================
  Files        273     273           
  Lines      23674   23675    +1     
=====================================
+ Hits       19478   19479    +1     
  Misses      3849    3849           
  Partials     347     347           

see 1 file with indirect coverage changes

@grcevski
Copy link
Author

grcevski commented Jan 9, 2025

Looks like the reservoir_test contains tests that exercise this code, but I'm happy to write more if you have a suggestion. I think the fix doesn't need a change log, but I'm happy to add an entry if you think otherwise.

@dashpole
Copy link
Contributor

dashpole commented Jan 9, 2025

Can you run benchmarks in sdk/metric to see if they demonstrate the improvement?

@dashpole dashpole added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 9, 2025
@dmathieu
Copy link
Member

And write a new benchmark if they don't?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants