Separate rewrite & recipe classpath, reload after changes - #463
Open
freya022 wants to merge 10 commits into
Open
Separate rewrite & recipe classpath, reload after changes#463freya022 wants to merge 10 commits into
freya022 wants to merge 10 commits into
Conversation
The `RewriteClassLoader` was cached per Gradle daemon and only replaced when the set of classpath locations changed. Recipes built by the project itself keep the same location when rebuilt, so their previous classes kept being used until the daemon was stopped. Compare the size and last modified time of each classpath entry as well, and shut down JGit's per class loader work queue thread when a class loader is replaced, as that thread would otherwise retain it.
To greatly reduce reloading classes during development by reusing most of rewrite's classes
To make sure the merged classpath is the same as before
So recipes from both the RewriteClassLoader and the recipe's CL can be discovered
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed?
Followup of Reload recipe classes when a classpath entry changed #456 which, in addition, creates a
ClassLoaderfrom the known rewrite dependencies, separated from the dependencies of therewriteconfiguration (a recipe's dependencies).The "rewrite" classpath consists of the known rewrite dependencies, but with their effective version.
The "recipe" classpath consists of the
rewriteGradle configuration, excluding dependencies present in the "rewrite" classpathWhat's your motivation?
Anything in particular you'd like reviewers to focus on?
RewritePluginTestinterface was not changed, it was simply moved toIRewritePluginTestbut Git fails to see it.RewritePluginTestto make sure that the effective classpath (how it was resolved before this PR) is the same as the combination of both rewrite + recipe classpaths, I've written the assertions inside the test's build script as usingProjectBuilderworked but was terribly inefficient/slow, let me know if there is a better way to test thisAny additional context
The previous PR mentioned there were issues with class loader leaks, however:
RewriteClassLoader, the changes did eliminate the leaking references, but the GC was quite lazy in reclaiming the memory, although the loader was correctly marked as reclaimable.Note that this is not a requirement, it is useful for testing, but G1 still reclaims memory in response to pressure, in other words, its not entirely broken, just very lazy
gradle.propertiesremoved, now using the default memory settings, and could not observe an OOM, unlike with the main branch which held multiple (>6) leaked class loadersWith these new changes,
rewriteDryRunafter a recipe update runs much faster compared to the previous PR, as most of rewrite's classes are reused, and only a fraction of the classes are reloaded, in my case, ~700 classes are loaded while taking 2.6 MB of metaspace (compared to ~8K and 52 MB)I will also note that, sometimes it is possible that the GC gets so lazy that Gradle will assume the Metaspace to be full before the JVM had a chance of cleaning it up, but it does not make the build fail - only the JVM will throw an OOM if there is an actual leakage - it will only make Gradle shut down after the build is finished.