-
Notifications
You must be signed in to change notification settings - Fork 1k
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
#5140: Tags merge optimization #4959
Conversation
Hello @jonatan-ivanov! Could you please review this MR? Thanks a lot in advance! |
Hello @shakuzen, could you please review this merge request when you have a chance? Thank you in advance! |
Thanks for the pull request. It's on our list of things to review. |
Thanks! Is there anything I can do to facilitate review, like maybe separating MR into 2 smaller independent MRs: one with benchmark and follow up with refactoring? So it may be easier to review? |
Hello there! May I kindly ask what is the status of review of this PR? May I somehow facilitate review making it happen any time soon? Thanks a lot in advance! |
32ced6c
to
dfbdde3
Compare
6b96b4d
to
e7a42c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the work on this. It's very appreciated. We just haven't had a chance to give it the attention it deserves yet. I'll be busy traveling for a conference through the end of next week, but I should be able to properly review and test this after that. I've left some initial quick feedback.
micrometer-core/src/main/java/io/micrometer/core/instrument/Tags.java
Outdated
Show resolved
Hide resolved
ad6ba40
to
18708d3
Compare
18708d3
to
05512c2
Compare
Updated benchmark results in PR description to latest master (+ extra benchmark) + proposed changes. |
@shakuzen could you please take another look at this MR once you have time? Thanks a lot in advance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement and added benchmarks. Sorry it took so long to get through review. We'll want to apply the same changes to KeyValues
which is essentially a copy of Tags
for use with the Observation
related API. I can take care of doing this so we don't need to further block getting this merged.
@shakuzen thank you very much for reviewing and accepting the PR! |
I've observed unexpectedly significant CPU time spent in Tags merge function in one of the production services which rely on micrometer:
I've slightly refactored internals of
Tags
class to take advantage of tags set is being already sorted array, which could be then combined with other sorted set in linear time.I've also added microbenchmark for tags concat operation to see the performance gain before I can test this change in production.
Below I'm providing results from
Apple M2 Pro
+JDK 21
machine:Updated benchmark results:
Baseline (b410174 + ac33ae9):
Full JMH output
Full JMH output:
With optimisations (05512c2):
Full JMH output:
Full JMH output: