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

Run Kotlinter in process isolation #420

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Run Kotlinter in process isolation #420

merged 2 commits into from
Dec 9, 2024

Conversation

jeremymailen
Copy link
Owner

@jeremymailen jeremymailen commented Dec 8, 2024

First in a series of changes leading to a 5.0.0 release. Starting with Kotlin 2.1.0 it's no longer acceptable to run ktlint and the embedded Kotlin compiler in the main Gradle classloader. The solution is to use Gradle worker process isolation.

Positive consequences:

  • Users can configure the version of ktlint
  • Kotlinter is compatible with any version of Kotlin with matching language specification

Side effects to watch:

  • Performance may be slower

First in a series of changes leading to a 5.0.0 release.
Starting with Kotlin 2.1.0 it's no longer acceptable to run ktlint and the embedded Kotlin compiler in the main Gradle classloader.
The solution is to use classloader Gradle worker isolation.

Positive consequences:
- Users can configure the version of ktlint
- Kotlinter is compatible with any version of Kotlin with matching language specification

Side effects to watch:
- Performance may be slower
- Potential [Gradle memory issues](gradle/gradle#18313)
This was referenced Dec 8, 2024
@jeremymailen
Copy link
Owner Author

Tested it on a substantial multi-module project with command gradle clean lintKotlin:

  • Before: 13s
  • After: 25s

So it will be slower, but this is the cost of isolation. Incremental of course is still fast and under 1s because generally not many files change between runs.

@jeremymailen
Copy link
Owner Author

We do experience the classloader leak bug however.
If I configure gradle.properties:

org.gradle.jvmargs=-Xmx1024m "-XX:MaxMetaspaceSize=1024m"

This helps quite a bit, but even then if you repeat clean lintKotlin several times it will run the gradle daemon out of metaspace. It may reclaim memory, but not aggressively enough.

@osipxd you seem to have some insight here. Is the workaround to use process isolation instead of classloader? It seems like we could run into different problems there.

@osipxd
Copy link

osipxd commented Dec 8, 2024

I haven't investigated possible workarounds deeply myself. But I've seen another possible workaround by @martinbonnin:

@jeremymailen jeremymailen changed the title Run Kotlinter in classloader isolation Run Kotlinter in process isolation Dec 9, 2024
@jeremymailen
Copy link
Owner Author

Tested it on a substantial multi-module project with command gradle clean lintKotlin:

  • Before: 13s
  • After: 25s

So it will be slower, but this is the cost of isolation. Incremental of course is still fast and under 1s because generally not many files change between runs.

Switching to process isolation yields:

  • After: 19s

So it's actually better than classloader isolation, but still a small performance hit.

@jeremymailen
Copy link
Owner Author

I haven't investigated possible workarounds deeply myself. But I've seen another possible workaround by @martinbonnin:

Thanks for the quick response. It actually looks like process isolation is not bad and it fixes the issue.

@jeremymailen jeremymailen merged commit 19ef1fe into master Dec 9, 2024
22 checks passed
@jeremymailen jeremymailen deleted the ktlint-isolation branch December 9, 2024 02:27
@martinbonnin
Copy link

FWIW, we went the process isolation route before but that created other questions like how many processes can you run in parallel? What heap size to allocate in each process, etc... It can work (and from the issues linked in gradle/gradle#18313, looks like this is definitely a solution) but it's no silver bullet either.

@martinbonnin
Copy link

martinbonnin commented Dec 12, 2024

FYI, there is a pull request to fix this 🎉 : gradle/gradle#31654

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

Successfully merging this pull request may close these issues.

3 participants