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

Add option to toggle Dokka Generator Worker API isolation #136

Merged
merged 14 commits into from
Jan 17, 2024

Conversation

martinbonnin
Copy link
Contributor

I have this locally for testing so opening the PR while I'm on this. Feel free to close if you think process isolation is better.

Reasons I like classloader isolation are:

  • I only need to monitor one JVM. In my build I had to bump the fork Xmx to > 2GB when my main build already has 8GB so theorically means the build could use 10GB or so.
  • It's easier to debug (no need to find the daemon and hook the debugger there)
  • Might be a bit faster too?

If the worker leaks then it becomes a problem but better fix the leaks?

@aSemy aSemy added the enhancement New feature or request label Jan 8, 2024
@aSemy aSemy force-pushed the use-classloader-isolation branch 2 times, most recently from b19970a to ad3c509 Compare January 9, 2024 17:49
@aSemy
Copy link
Contributor

aSemy commented Jan 9, 2024

Thanks! Classloader isolation should be better in theory.

I've tidied up your suggestion and allowed for it to be configurable. Unfortunately I got a lot of test failures due to metaspace limits when using classloader isolation, but perhaps that's not a problem - the tests in this project are quite atypical, and run many different projects. Maybe the build/JUnit config needs to be updated to fork the process for each test suite?

@martinbonnin
Copy link
Contributor Author

due to metaspace limits when using classloader isolation

Sounds like a classloader leak somewhere :/. Maybe those coroutine dispatcher threads holding onto classloader references or so.

@aSemy
Copy link
Contributor

aSemy commented Jan 15, 2024

Status update:

I haven't seen any more memory issues. Perhaps it was some Gradle/Windows issue (e.g. gradle/gradle#23959 gradle/gradle#23965). So I'm happy to proceed.

I want to have at least one test (using Gradle TestKit) first though. Maybe it's possible to parameterise an existing test to toggle between classloader/process isolation. I can pick this up, but it might take some time. Contributions to speed this up would be welcome :)

I also wonder what might happen in a multimodule project, if some subproject use process isolation and others use classloader isolation. Probably it still works. I wonder if performance will be better with process isolation? Or does that require a shared Build Service so that Gradle will re-use the worker queue?

@martinbonnin
Copy link
Contributor Author

if some subproject use process isolation and others use classloader isolation. Probably it still works.

I think it'll still work but TBH I don't expect that to be a big use case. Most of the users will use the default. And the ones who change them would IMO change it everywhere.

Contributions to speed this up would be welcome :)

No promises but I'll try to look into this!

@martinbonnin
Copy link
Contributor Author

Attempt at a test here. I'm no kotest expert so not sure if there's a more idiomatic way to do this but this seemed to run the tests at least.
It doesn't do any assertion on classloader vs process isolation. Not sure how to test this.

@aSemy aSemy changed the title proposal: Use classloader isolation instead of process isolation Add option to toggle Dokka Generator Worker API isolation Jan 16, 2024
@aSemy
Copy link
Contributor

aSemy commented Jan 16, 2024

Thanks for this! I really appreciate it, I've finished it off.

I've made some tweaks (the Isolation mode is configured via the Dokkatoo extension, added some docs).

I moved your test to the multi-module test just to make the test more thorough, and added some assertions to make sure the isolation mode was set properly.

@aSemy aSemy added this to the v2.1.0 milestone Jan 16, 2024
@martinbonnin
Copy link
Contributor Author

Thanks! Good trick about asserting on the logs 👍

@aSemy aSemy enabled auto-merge January 17, 2024 21:21
@aSemy aSemy added this pull request to the merge queue Jan 17, 2024
Merged via the queue into adamko-dev:main with commit 5cd7d18 Jan 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants