-
Notifications
You must be signed in to change notification settings - Fork 165
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
[JENKINS-59587] Use weakValues on the inner caches in SandboxResolvingClassLoader.parentClassCache instead of on the outer cache #265
Conversation
…entClassCache instead of on the outer caches
} | ||
|
||
return builder.build(parentLoader -> Caffeine.newBuilder()./* allow new plugins to be used, and clean up memory */expireAfterWrite(Duration.ofMinutes(15)).build()); | ||
return outerBuilder.build(parentLoader -> innerBuilder.build()); |
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.
Note that reusing innerBuilder.build
should be safe according to its Javadoc.
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.
AFAICT
...in/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoader.java
Show resolved
Hide resolved
One other thing that seemed a bit concerning in the thread dumps I saw was that some class loading threads held locks on a Looking into the code, I think Example thread holding a
Example thread blocked waiting for that
|
I enabled statistics collection so that we can easily evaluate cache performance through the script console with a snippet like this:
|
|
Can this be done via |
...ava/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SandboxResolvingClassLoaderTest.java
Show resolved
Hide resolved
Sure, now that we are recording stats, we could hook them up to whatever extension points might use them, although I'd prefer to not add any dependencies to this plugin if possible. |
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.
If CI passes!
I explored the things mentioned in #265 (comment) in #271. In summary, unless JENKINS-23784 was addressed, using an asynchronous cache makes no difference, so I think it makes sense to keep using the manual cache for now. |
See JENKINS-59587.
While investigating a user-reported performance issue, I noticed among other things that their Pipelines were spending a ton of time blocked waiting to acquire the lock for
PluginManager$UberClassLoader
while loading classes inSandboxResolvingClassLoader
. This was a little surprising, because this user has a single shared library that is used by all of their Pipeline builds, so I would've thought that they would have a very high cache hit ratio, but that did not seem to be the case.Looking into it, I noticed that in
SandboxResolvingClassLoader
, we were usingweakValues
onparentClassCache
itself as of #252. This didn't make sense to me, because the values ofparentClassCache
areCache<String, Class<?>>
, and from what I can tell, these caches are only referenced internally byparentClassCache
, which meant that the inner caches would always be removed when the garbage collector runs, making the cache not very useful when Jenkins is under significant memory pressure (which was definitely case for this user for other reasons).This PR switches to using
weakValues
onparentClassCache
's inner caches, rather thanparentClassCache
itself. This way, the classes referenced by the inner cache are still only weakly referenced, so that their class loader, which may itself be a key ofparentClassCache
, is still able to be garbage collected. Looking into it some more, I think #252 was doing this already before 36d3b3a, but that commit changed the semantics fromCache<ClassLoader, Cache<String, Optional<WeakReference<Class<?>>>>>
toCache<ClassLoader, WeakReference<Cache<String, Optional<Class<?>>>>>
, and unfortunately we didn't notice that change in semantics during review.I added a basic smoke test that fails if you revert the changes in
src/main
.GroovyMemoryLeakTest
still passes, but I'm not sure what other testing would be useful to make sure this doesn't cause any regressions.