-
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
Fix performance regression in MeterRegistry#remove #5750
Conversation
Adds a reverse look-up for the pre-filter meter ID for use when removing a Meter. This avoids the need to iterate over the meters in the cache (preFilterIdMeterMap), which scales linearly with the number of meters, and is problematic because it does this while holding the meterMap lock needed to add new meters. Also adds benchmarks for measuring the performance of the remove method with different numbers of meters registered.
Benchmark results demonstrating the issue: Micrometer 1.14.2:
With the changes in this pull request:
(I don't know why it's relatively slower with less meters) |
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 quick fix. 👍👍
@Benchmark | ||
@Warmup(iterations = 100) | ||
@Measurement(iterations = 500) | ||
@BenchmarkMode(Mode.SingleShotTime) |
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.
Should we add a line that unlike Sample
this measures this calls the method once and also measures the "cold" performance?
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 not necessarily the cold performance we care about, but by the mutable nature of a remove
method, we want to measure the performance of removing a meter that exists in the registry when the registry has a certain number of total meters, and I didn't know a good way to do that other than single shot mode. In other modes, we would either be measuring removing a meter from a registry with progressively less meters in total, or we would be measuring the time to remove a non-existent meter, which isn't what we want. I've added reasoning in a JavaDoc comment now.
I guess since we "only" doubling the references and not the objects, the memory impact is not too bad. |
That is my expectation, but we don't track/test this now. If anyone has good ideas on doing that, please share. |
Adds a reverse look-up for the pre-filter meter ID for use when removing a Meter. This avoids the need to iterate over the meters in the cache (preFilterIdMeterMap), which scales linearly with the number of meters, and is problematic because it does this while holding the meterMap lock needed to add new meters. Also adds benchmarks for measuring the performance of the remove method with different numbers of meters registered.
Resolves #5466