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

FISH-8688 improvement: optimize MR-jar class loading #6713

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented May 10, 2024

Description

Optimize multi-release JAR processing, remove resource leak from MR-JarFile remaining open
Opening MR-Jars correctly by using RUNTIME_VERSION
fixes #6554

Testing Performed

Deploy original reproducer for this issue

Testing Environment

Any

@lprimak lprimak force-pushed the optimize-mr-jar branch 3 times, most recently from ce2aa07 to 7456a0e Compare May 13, 2024 04:23
@lprimak lprimak changed the title bugfix: optimize MR-jar processing improvement: optimize MR-jar processing May 13, 2024
@lprimak lprimak changed the title improvement: optimize MR-jar processing improvement: optimize MR-jar ClassLoading May 13, 2024
@lprimak lprimak changed the title improvement: optimize MR-jar ClassLoading improvement: optimize MR-jar class loading May 13, 2024
@Pandrex247 Pandrex247 changed the title improvement: optimize MR-jar class loading FISH-8688 improvement: optimize MR-jar class loading May 13, 2024
@Pandrex247 Pandrex247 added the PR: CLA CLA submitted on PR by the contributor label May 13, 2024
@RInverid
Copy link

I get a deployment failure with this PR - an @DependsOn annotation fails to resolve another singleton in the same ejb package. (the singleton that is depended on implements an interface, and that interface is injected into the singleton with the @DependsOn annotation)

@lprimak
Copy link
Contributor Author

lprimak commented May 23, 2024

@RInverid do you have a reproducer or at least a stack trace? There isn’t much to go on in your comment

@RInverid
Copy link

Not yet, but I have more practical feedback: this MR does not load Bouncycastle libs if running on JDK 17. BC libs are multirelease, but they only have class files for jdk9. Your PR skips loading BC classes becuase it cannot find classes for jdk17

@lprimak
Copy link
Contributor Author

lprimak commented May 28, 2024

Hmmmm. I specifically tested BC libs in 11 (but not 17) so I am not sure how 17 would be any different.
Looking forward to a reproducer.

@Pandrex247
Copy link
Member

Had a quick look myself, I think you can use the reproducer provided against the original change PR to see the error (albeit against embedded): https://github.com/ctabin/gf-test-lucene

Without this PR, it works as it's able to load the Analyzer class.
With this PR, issues start arising again with it unable to find the Analyzer class

Not yet determined the failing difference Yet™

@lprimak
Copy link
Contributor Author

lprimak commented Jun 3, 2024

Ok, I didn't realize that commit was not only optimization but a bug fix. I re-incorporated the bug fix into this PR and tested it with https://github.com/ctabin/gf-test-lucene and it works

@RInverid
Copy link

RInverid commented Jun 3, 2024

Thanks for the changes, I re-tested this MR. The deployment time and memory Issues are seeing are less of an issue, but still present. Probably because the problematic change (JarFile jar = new JarFile etc) is still called, but significantly less often.

@lprimak
Copy link
Contributor Author

lprimak commented Jun 3, 2024

@RInverid how about now?

@lprimak
Copy link
Contributor Author

lprimak commented Jun 3, 2024

Jenkins test

@lprimak
Copy link
Contributor Author

lprimak commented Jun 3, 2024

Jenkins test please

@RInverid
Copy link

RInverid commented Jun 4, 2024

Yes this resolves my issues, as I've also found with my testing. Not sure what the change in ProtectedJarFile does though, its at least not necessary to resolve my issues according to my testing.

@Pandrex247 Pandrex247 merged commit 7021d83 into payara:master Jun 7, 2024
1 check passed
@Pandrex247
Copy link
Member

Thanks both! 👌

@lprimak lprimak deleted the optimize-mr-jar branch June 7, 2024 08:29
Pandrex247 added a commit to Pandrex247/Payara that referenced this pull request Jun 19, 2024
FISH-8688 improvement: optimize MR-jar class loading
Pandrex247 added a commit to Pandrex247/Payara that referenced this pull request Jun 26, 2024
FISH-8688 improvement: optimize MR-jar class loading
Pandrex247 added a commit to Pandrex247/Payara that referenced this pull request Jul 2, 2024
FISH-8688 improvement: optimize MR-jar class loading
Pandrex247 added a commit to Pandrex247/Payara that referenced this pull request Jul 5, 2024
FISH-8688 improvement: optimize MR-jar class loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
3 participants