-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Reduce allocation in matching of unknown properties #44448
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I'm a bit worried about the test failures. But unfortunately there's nothing in the log about the 500 errors that the client encounters. |
ce6bf7e
to
5888ce8
Compare
I asked for a rebase. |
Yes, I've executed this locally and got the same error. Apparently, they are related. I'll have a look. |
5888ce8
to
fce77b1
Compare
The issue was caused by the runtime property I did change the logic in the recording to skip check in other type phases once a match is found, but we need to check all phases. |
This comment has been minimized.
This comment has been minimized.
I think the native test failing is probably still a problem. |
Didn't notice. Let me have a look. |
fce77b1
to
a04c136
Compare
The issue was caused by |
Status for workflow
|
Failing tests are just flaky now. |
public static Set<PropertyName> toPropertyNames(final Set<String> names) { | ||
Map<PropertyName, String> propertyNames = new HashMap<>(); | ||
for (String name : names) { | ||
PropertyName propertyName = new PropertyName(name); |
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.
What if...PropertyName
become a mutable class (which can wrap different names) and we allocate a single instance of it - if it is often NOT added?
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.
We only need this at start to validate names. Adding is only done once. My plan was to write a proper API for this in SR Config.
Map<PropertyName, String> propertyNames = new HashMap<>(); | ||
for (String name : names) { | ||
PropertyName propertyName = new PropertyName(name); | ||
if (propertyNames.containsKey(propertyName)) { |
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.
Depending what's the actual common case you want to optimize for and the number of names, you can just use remove
checking for null
, to save containsKey
+ remove
to happen.
If this one is not the common scenario, instead, you can just perform a propertyNames.putIfAbsent
and after checking that's not null
, handling the subsequent remove
/change
/addition
logic
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 is not exactly to optimize, but because different String
may be equals (for instance, when they match *
), then we need to give priority to the *
name when inserting it into the collection.
} | ||
} | ||
unknownProperties.removeAll(toRemove); | ||
return propertyNames.keySet(); |
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 know that sounds paranoid (it is a bit I admit), but beware keySet
if the lifecycle of the String
used as values is to go away, the key set within HashMap
retain a strong reference to the original map (including the entries and values) preventing the String
s to be GCd - see https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/HashMap.java#L989 - the hidden this
due to be an inner class
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.
We can rewrite it, but I'm not very concerned. This is used only during build time, and the method calls are all local when validating / generating the configuration, so everything should be GC by the end of the call.
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.
Let's get this in, we can still improve later.
I think we will have to backport it to include it in 3.17 as it's fixing a regression. |
Use
io.smallrye.config.PropertyName
instead ofio.smallrye.config.KeyMap
.io.smallrye.config.PropertyName
requires less allocations and it is faster when matching property names, because it does not need to split theString
.micrometer-prometheus
integration test after moving Kubernetes to @ConfigMapping #44216