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

Replace usage of NeverInline in jdbc-pgsql #27960

Merged
merged 2 commits into from Sep 19, 2022

Conversation

jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Sep 15, 2022

The previous implementation relied on a graal compiler optimization to never inline a method. By doing so it tricked native-image into not including XML processing code when it's not used.

Instead, use a DomHelper-local bifunction field which stays null unless the XML processing code is actually reachable. If it is, the bifunction is being set by the reachability handler and, thus, achieves the same result.

Closes #27791

The previous implementation relied on a graal compiler optimization to
never inline a method. By doing so it tricked native-image into not
including XML processing code when it's not used.

Instead, use a DomHelper-local bifunction field which stays null unless
the XML processing code is actually reachable. If it is, the bifunction
is being set by the reachability handler and, thus, achieves the same
result.

Closes quarkusio#27791
@jerboaa
Copy link
Contributor Author

jerboaa commented Sep 15, 2022

I'm testing this with the mandrel GHA suite here: https://github.com/graalvm/mandrel/actions/runs/3060671324

@jerboaa
Copy link
Contributor Author

jerboaa commented Sep 15, 2022

Mandrel GHA shows no new failures (we were seeing failures in Data5).
https://github.com/graalvm/mandrel/actions/runs/3060671324/jobs/4939876284

Remaining failures are all related to the kotlin issue. See #27300.

Example failure without this patch from our nightlies:
https://github.com/graalvm/mandrel/actions/runs/3057751522/jobs/4933401387

Copy link
Contributor

@zakkak zakkak left a comment

Choose a reason for hiding this comment

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

LGTM. Nice approach @jerboaa!

@jerboaa
Copy link
Contributor Author

jerboaa commented Sep 19, 2022

LGTM. Nice approach @jerboaa!

Thanks for the review!

@gastaldi gastaldi merged commit 5d758ec into quarkusio:main Sep 19, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Sep 19, 2022
@gastaldi
Copy link
Contributor

@jerboaa @zakkak should we backport this to 2.13?

@zakkak
Copy link
Contributor

zakkak commented Sep 19, 2022

@jerboaa @zakkak should we backport this to 2.13?

Yes please :)

@Sanne
Copy link
Member

Sanne commented Sep 21, 2022

Very nice pattern, I didn't know we could do that. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JPAFunctionalityInGraalITCase.verifyJdkXmlParsersHavebeenEcludedFromNative test fails with GraalVM 22.3.0-dev
5 participants