-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Get rid of a lambda in Arc runtime code #25555
Conversation
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.
LGTM
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.
TBH I'm not a big fan of similar changes. How big is the impact? I mean what do we get? Is it at least a millisecond?
} | ||
} | ||
nonSuppressed.sort(PRIORITY_COMPARATOR); | ||
return Collections.unmodifiableList(nonSuppressed); |
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.
Hm, this is not equivalent in the sense that previously an optimized immutable list (List.of()
) was used whereas here we use an unmodifiable wrapper...
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 equivalent for all intents and purposes, no?
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 equivalent in terms of memory consumption because in theory there will be a lot resolved lists of beans that are stored in the cache and Collections.unmodifiableList(new ArrayList<>())
is definitely worse compared to List.of()
.
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.
But that List.of
is produced by a Stream
which itself takes up memory to be processed.
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.
But that
List.of
is produced by aStream
which itself takes up memory to be processed.
Which is freed afterwards but the list is there to stay as a cached value.
I was thinking about replacing the Collections.unmodifiableList(nonSuppressed)
with List.copyOf(nonSuppressed)
...
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.
No problem, done
Well, we have agreed to not include lambdas in runtime code, correct? Measuring differences at the millisecond level is likely close to impossible. |
These are not lambdas but method references ;-)
My point is that we do crazy stuff like this to save some unmeasurable amount of time but then some extensions take half a second to start :-( |
True of course, but their invocation is still handled at runtime, so they do have an overhead compared to classes :)
Yeah I understand, but in this case there is a known issue that the JVM thrown an exception in order to handle the control flow (which is bad for performance but there is nothing we can do to influence the JVM to do otherwise) and the fix is very easy, so I don't see the reason why to not apply it. |
These are causing NoSuchMethodError on application startup (this can be verified easily either via the debugger, or from JFR events - see https://bugs.openjdk.java.net/browse/JDK-8161588 for more details about why the error occurs) which can have a small impact on startup performance N.B. I only replaced the code for the instances that were causing the issue at the startup of a Quarkus application
8f15a1f
to
5abb012
Compare
These are causing NoSuchMethodError on application startup
(this can be verified easily either via the debugger, or from JFR events - see
https://bugs.openjdk.java.net/browse/JDK-8161588 for more details
about why the error occurs) which can have a small impact on startup performance
N.B. I only replaced the code for the instances
that were causing the issue at the startup of a
Quarkus application