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
Fix memory leak with named instantiator #8153
Conversation
This commit reworks the named object instantiator so that it doesn't leak memory, by locating the cache on the `VisibleURLClassLoader` instance whenever possible. Previously, it retained strong references to classes, meaning that any class loaded via a script classloader would be strongly referenced and would prevent the loader from being collected. With this change, the cache is now located on the loader itself. If, for some reason, the loader is not a known classloader, we will still use a global cache, but this shouldn't leak memory anymore since in this case it's likely the affected classes come from Gradle core itself. Fixes #8142
5efc061
to
20c5114
Compare
@@ -33,6 +36,28 @@ | |||
} | |||
} | |||
|
|||
private final EnumMap<UserData, Object> userData = new EnumMap<UserData, Object>(UserData.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭕️ I think an enummap + synchronized here is not any better than using a concurrent hash map. I'd go with the latter, so we don't need to keep an enum of keys here, giving us better separation of concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EnumMap
has the main advantage of having descriptive keys and optimized lookup/size. Also a ConcurrentMap
will not prevent the same value from being computed concurrently, which we want to avoid (or we could have race conditions and linkage errors).
|
||
private LoadingCache<Class<?>, LoadingCache<String, Object>> newValuesCache() { | ||
return CacheBuilder.newBuilder() | ||
.weakKeys() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Weak keys aren't necessary here (and won't help either)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, leftover of previous implementation.
This is unnecessary at this point.
Context
See #8142