Skip to content
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

Merged
merged 1 commit into from
May 13, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented May 13, 2022

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

@geoand geoand requested a review from mkouba May 13, 2022 08:51
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label May 13, 2022
Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mkouba mkouba left a 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);
Copy link
Contributor

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...

Copy link
Contributor Author

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?

Copy link
Contributor

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().

Copy link
Contributor Author

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.

Copy link
Contributor

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.

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)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, done

@geoand
Copy link
Contributor Author

geoand commented May 13, 2022

Well, we have agreed to not include lambdas in runtime code, correct?

Measuring differences at the millisecond level is likely close to impossible.
But these changes (I've started doing them elsewhere as well in cooperation with @franz1981) do add up.
Moreover, they cut down and on the false warnings Java Mission Control reports when analyzing a Quarkus application's JFR events

@mkouba
Copy link
Contributor

mkouba commented May 13, 2022

Well, we have agreed to not include lambdas in runtime code, correct?

These are not lambdas but method references ;-)

Measuring differences at the millisecond level is likely close to impossible.
But these changes (I've started doing them elsewhere as well in cooperation with @franz1981) do add up.
Moreover, they cut down and on the false warnings Java Mission Control reports when analyzing a Quarkus application's JFR events

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 :-(

@geoand
Copy link
Contributor Author

geoand commented May 13, 2022

These are not lambdas but method references ;-)

True of course, but their invocation is still handled at runtime, so they do have an overhead compared to classes :)

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 :-(

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
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 13, 2022
@quarkus-bot

This comment has been minimized.

@geoand geoand merged commit bd3b19f into quarkusio:main May 13, 2022
@geoand geoand deleted the arc-runtime-lambda branch May 13, 2022 18:14
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 13, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 13, 2022
@gsmet gsmet modified the milestones: 2.10 - main, 2.9.1.Final May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants