-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Use IdentityHashMap for consistent JIT optimization in AbstractCompositeMeter #6846
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
base: main
Are you sure you want to change the base?
Use IdentityHashMap for consistent JIT optimization in AbstractCompositeMeter #6846
Conversation
7ea05ba to
3edc0a9
Compare
...meter-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java
Outdated
Show resolved
Hide resolved
a55c2da to
1e39d3f
Compare
|
@jonatan-ivanov - Made suggested change. Confirmed the allocations have stopped: #6811 (comment) |
|
@belugabehr could you share the code for the benchmark you're using? |
shakuzen
left a comment
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 this change is fine as is, but it's also brittle. I wonder if we should try adding a test that asserts there is no allocation after C2 compilation. The problem is that we only have heuristics to guess when C2 compilation is done - by default in most modern JVMs it starts after 10,000 invocations of a method.
It may be helpful to commit the benchmarks along with this, but benchmarks don't get run with the build so they are not a guarantee this won't regress with some other change.
I think there is larger refactoring to consider around this, but I think it's okay to consider that after merging this.
I also think we should pursue adding a warning log so that users who can do all setup before registering metrics can avoid surprises like this and others (such as the values being recorded before a non-composite registry is added being dropped).
|
|
||
| private Map<MeterRegistry, T> children = Collections.emptyMap(); | ||
| @SuppressWarnings("unchecked") | ||
| private Map<MeterRegistry, T> children = (Map<MeterRegistry, T>) EMPTY_CHILDREN; |
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.
If we want to ensure a specific type for optimizations, it's probably best to use the specific type on the left-hand side here.
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.
Static values like this cannot be templated and require this bridge. Please consider this PR as-is. If you would like a different variation, would you mind creating a new PR with whatever you think is best in order to streamline the back-and-forth? Thanks!
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.
| private Map<MeterRegistry, T> children = (Map<MeterRegistry, T>) EMPTY_CHILDREN; | |
| private IdentityHashMap<MeterRegistry, T> children = (IdentityHashMap<MeterRegistry, T>) EMPTY_CHILDREN; |
My comment wasn't clear enough. I should have used the code suggestion feature, but other changes are required along with it. Anyway, we can take care of it as polish when merging. I thought it was worth bringing up in review, though.
Agreed! |
I've opened #6908 for this. |
…iteMeter Replace Collections.emptyMap() with new IdentityHashMap<>() to maintain consistent map implementation type throughout the object's lifecycle, enabling JVM scalar replacement optimization and reducing iterator allocation overhead. Fixes micrometer-metricsgh-6811 Signed-off-by: David Mollitor <[email protected]>
1e39d3f to
23bd25c
Compare
|
@shakuzen - PR updated with your suggestions. Thanks for all the help! Excited to get this over the line. |
Replace Collections.emptyMap() with new IdentityHashMap to maintain consistent map implementation type throughout the object's lifecycle, enabling JVM scalar replacement optimization and reducing iterator allocation overhead.
Fixes gh-6811