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

[Gradle] Use processIsolation instead of classLoaderIsolation #746

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

ru5j4r0
Copy link
Contributor

@ru5j4r0 ru5j4r0 commented Mar 6, 2024

Hi.
First of all, I appreciate your great work.
This is my first PR for the Play projects, so sorry in advance if I'm doing something wrong.

I was trying to migrate a few Play Framework projects to Gradle, but I couldn't migrate some of them because compileTwirl task gave me an out of memory error of JVM's Metaspace.
I was able to migrate a project that has dozens of views, but I couldn't which has around 1,000 views.
My PC has 32GB RAM, but increasing Xmx nor MaxMetaspaceSize didn't fix the issue.

After searching for a bit, I found this Gradle's issue.
I forked the Twirl repo, replaced classLoaderIsolation with processIsolation, published it to mavenLocal() and using that SNAPSHOT version of the Twirl plugin from my local Maven repo actually fixed the problem.

I just filed a PR right away because it's a small change.
What are your thoughts on this change?

Possibly related

Other projects doing the same fix

@ru5j4r0 ru5j4r0 changed the title Use processIsolation instead of classLoaderIsolation Use processIsolation instead of classLoaderIsolation in Gradle plugin Mar 6, 2024
@mkurz mkurz requested a review from ihostage March 7, 2024 10:36
@ihostage ihostage self-assigned this Mar 7, 2024
@ru5j4r0
Copy link
Contributor Author

ru5j4r0 commented Mar 16, 2024

I suppose the drawback of using processIsolation is a increased overhead due to the need to start a new process?
Perhaps I can add an option to choose the isolation method, in case replacing classLoaderIsolation with processIsolation by default is not acceptable.

@ihostage
Copy link
Member

Hi, @ru5j4r0!

I suppose the drawback of using processIsolation is a increased overhead due to the need to start a new process?

It's okay, don't worry. I will investigate this issue in more detail next week.

Copy link
Member

@ihostage ihostage left a comment

Choose a reason for hiding this comment

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

Unfortunately, I couldn't find a way to write a unit test for this case 😞
But I've made a lot of experiments locally and I agree with the proposed changes.
Thank you very much, @ru5j4r0, and apologies for the delay!

@ihostage ihostage changed the title Use processIsolation instead of classLoaderIsolation in Gradle plugin [Gradle] Use processIsolation instead of classLoaderIsolation Apr 24, 2024
@ihostage ihostage merged commit 5b386d3 into playframework:main Apr 24, 2024
15 checks passed
@ru5j4r0
Copy link
Contributor Author

ru5j4r0 commented Apr 26, 2024

@ihostage
I’m the one who owes you an apology for opening a PR without tests or reproducible examples. Thank you for all your work!

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

Successfully merging this pull request may close these issues.

2 participants