-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improved Meter.Id#getTags() performance #6182
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
Improved Meter.Id#getTags() performance #6182
Conversation
| public List<Tag> getTags() { | ||
| List<Tag> tags = new ArrayList<>(); | ||
| this.tags.forEach(tags::add); | ||
| if (this.tags == Tags.empty()) { |
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.
It would be better to explicitly check for zero length, as it's possible to construct another instance with zero tags, but we don't have that API.
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.
I think it's worth adding API for size/length to Tags. Given this is in the same package, we can make it package private. We could consider separately whether to make it public API if there's good justification for it.
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.
it's possible to construct another instance with zero tags
I'd also note it's not possible with public API, so as long as we don't inadvertently do it, we shouldn't need to worry about that case.
micrometer-core/src/main/java/io/micrometer/core/instrument/Meter.java
Outdated
Show resolved
Hide resolved
Signed-off-by: etki <[email protected]>
674a6e4 to
b3fa190
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.
I updated the PR with adding a package-private size method on Tags so we can exactly size the ArrayList instead of guessing. Feel free to run the benchmarks again if you have spare time and desire, but I think it's safe to say it should strictly be better this way.
We also have use for the size() API for similar improvement in DefaultMeterObservationHandler#createTags (there it would be on KeyValues), but I'll defer considering a change that requires making a new public method to outside this PR so we don't block these changes from being merged.
Hey. This is a very small and simple PR aimed at a tiny performance improvement: currently, the .getTags() call always allocates a zero-sized array list and then uses a .forEach call (which results in allocating another object for lambda, and, in the worst case, non-inlined call) to populate it. PR checks if list is needed at all, and if so, asks for a list with a space for 32 elements, populating it in a classic loop. The number of 32 is only a guess with no real grounds.
This is definitely not the biggest problem around, so the change is perceptible, yet not a game changer.
Benchmarks from #6174 and Intel N100 fixed at 2GHz were used to estimate the impact. The benchmark uses identifiers with 0-64 tags, where the number of tags in the
modecolumn occurs 90% of time, and other values are distributed evenly in the remaining space.43.379 ± 0.05644.352 ± 0.78271.544 ± 0.18865.575 ± 0.48081.493 ± 0.70472.908 ± 1.04892.932 ± 0.45182.054 ± 1.028120.026 ± 1.511104.113 ± 1.794209.666 ± 2.159143.756 ± 3.422345.139 ± 3.130244.574 ± 7.125618.301 ± 6.336496.224 ± 14.074Please note that some improvements suggested in #6113 (using
Collections.unmodifiableList(Arrays.asList(...).subList(...)or even creating a custom List implementation that would combine all the three) will likely bring these numbers down to tens of ns per call on arrays of any length and make this PR completely redundant.