classloader-leak-prevention icon indicating copy to clipboard operation
classloader-leak-prevention copied to clipboard

improvement: add configuration to avoid System.gc even if -XX:+DisableExplicitGC is not present

Open matlach opened this issue 8 years ago • 3 comments

Hi @mjiderhamn ,

I'd like to leverage your excellent library though I'm not confortable with the System.gc that will be triggered if the -XX:+DisableExplicitGC jvm switch as not been turned on.

This is done in these 2 classes:

  • https://github.com/mjiderhamn/classloader-leak-prevention/blob/master/classloader-leak-prevention/classloader-leak-prevention-core/src/main/java/se/jiderhamn/classloader/leak/prevention/ClassLoaderLeakPreventor.java
  • https://github.com/mjiderhamn/classloader-leak-prevention/blob/master/classloader-leak-prevention/classloader-leak-prevention-core/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/StopThreadsCleanUp.java

Here's the use case:

  • My company run performance testing with multiple sites deployed on a single application server.
  • Deploy and undeploy operation can happen at any time.
  • Forcing a GC on each undeploy operation could potentially have performances impact on the other sites deployed.

I think, solely relying on the -XX:+DisableExplicitGC is a bit problematic. Sure I could most likely extends your library to change the behavior though, I feel it would be a great addition if we could instead leverage a configuration to explicitly enable or not the System.gc.

Maybe a bit like the other "ClassLoaderLeakPreventor. ..." configuration you already provide (through the context param).

I also think, it could be worthwile to enable/disable any individual leaks prevention feature one by one. So when hunting down for leaks and fixing them, it could be possible to still leverage your library to solve one issue at the time.

WDYT?

Again a big thank you for your excellent library.

matlach avatar Aug 12 '16 18:08 matlach

Hi Mathieu and thank you!

Despite your explanation, I don't quite see why you need this configuration. What is the problem with -XX:+DisableExplicitGC? Do you need to do explicit GC in other circumstances, but avoid it only in the leak preventor?

Also, did you notice under what circumstances the leak preventor does explicit GC? Currently there are only two cases when this will be used. The first (in StopThreadsCleanUp) is if your app includes Open Office Java Uno RunTime (JURT). The second (in ThreadGroupCleanUp) will happen if any custom ThreadGroups were created in your app. This does not seem to be very common. The only third party library I've seen that uses ThreadGroups is JGroups (which is a dependency of Infinispan).

Given that information, do you still think explicit GC will be a problem in your scenario...?

Furthermore, I'm not completely convinced that explicit GC will have a bigger impact on the performance results, than the fact that multiple other applications are running in parallell (and implicit GC). Although that also depends on what Garbage Collector is used (Parallell would likely have much more impact than CMS or G1). What GC are you using?

mjiderhamn avatar Aug 16 '16 07:08 mjiderhamn

Sorry for the delay, I was in vacation without Internet connectivity.

We are indeed using JGroups through Infinispan / Wildfly Application Server. We are currently using CMS + ParNewGC though will try to migrate to G1GC soon especially to gain the benefits of the -XX:+UseStringDeduplication.

Correct me if I'm wrong, though, when we enable the -XX:+DisableExplicitGC, we also loose the ability to trigger a GC directly from the JVisualVM and/or JConsole? When running performance testing, we would like to keep this ability. This is also why having a dedicated configuration other than relying on -XX:+DisableExplicitGC would be handy.

You're probably right that this is a 'corner case' though I still feel this is fair feature request. I'm willing to implements the configuration through a pull request if you are willing to merge it after it satisfy your/the library quality requirements.

Thanks!

matlach avatar Aug 22 '16 15:08 matlach

I've been thinking on this for a bit. For ThreadGroupCleanUp I think we could add configuration to avoid explicit GC, but the documentation should include a warning that then we can't predict when stale java.beans.ThreadGroupContext entries will be expunged. (One potential improvement is to trigger stale expunging also on startup, in case implicit GC happens between undeploy and redeploy.)

For the JURT case, I cannot yet see a way to shut it down properly without first forcing GC. Do you?

mjiderhamn avatar Aug 28 '16 18:08 mjiderhamn