pojomatic icon indicating copy to clipboard operation
pojomatic copied to clipboard

POJOMATORS a potential memory leak

Open jglick opened this issue 10 years ago • 2 comments

Pojomatic statically caches the handler for each Class. This could produce a memory leak in case you run it on a Class in a ClassLoader which is later thrown away.

Using WeakHashMap unfortunately does not help, since values refer to keys, and apparently there is no perfect solution. See my JDK-6389107; probably the best you can do is to hold the value with a SoftReference.

jglick avatar Oct 01 '14 20:10 jglick

This is actually something I've worried about for awhile, but haven't tackled precisely because it's a hairy problem. I'm wondering right now if it could be solved by having each Pojomator hold only a WeakReference to the pojomated class, and have Pojomatic use a WeakHashMap.

There is another leak that will occur, since the DynamicClassLoader will still have references to the generated Pojomator classes. However, if the pojomator classes only have weak references to their pojomated classes, then this leak shouldn't prevent a different classloader from being GCed. There are, I believe, only two ways to avoid this:

  1. Create a separate classloader for each pojomator. This replaces a slow memory leak with a memory hog, and hardly seems like a good trade-off.
  2. Create a separate DynamicClassLoader for each classloader that pojomatic deals with. This would have a couple of benefits:
  • With the right care, garbage collection of the classloader defining a pojomated bean could make the corresponding DynamicClassLoader elligible for collection
  • When a pojomated field or getter is public, we could bypass invokeDynamic and refer directly to the field or method, enabling possible performance improvements.

I'll start thinking about whether any of these techniques can actually work.

irobertson avatar Oct 01 '14 20:10 irobertson

There's a relatively cheap way to address this, which is to add a method to Pojomatic, say, uncachedPojomator, which creates a Pojomator for a class, but does not cache it. A class one desires to be removable could then add a private static Pojomator field, initialized on class load, and use that for the equals/toString/hashCode methods.

Unfortunately, there's a hitch - whenever Pojomator.doEquals is passed an instance of a type other than the original type for the Pojomator, it needs to do a compatibilty-for-equals check. It does this by looking at an org.pojomatic.internal.ClassProperties instance for the other class. For performance reasons, this instance is also cached. We could ameliorate this by caching ClassProperties instances not statically, but on a per-pojomator basis. There might a be a small amount of duplication, but unless a wide range of pojomated classes are checked against each other, the cost wouldn't be great.

It's worth noting that this would approach still result in a pojomated class hanging onto references to any classes that it generated ClassProperties instances for. Fortunately, this would only happen for classes living in the same hierarchy; I suspect that in most cases, such classes also come from the same ClassLoader, so the real-world impact of this limitation would likely be small.

Lurking somewhere in the background of all this is a related concern, which is that I suspect (though have not verified) that Pojomatic may be unable to work on classes which come from a ClassLoader unknown to Pojomatic's ClassLoader.

irobertson avatar Nov 17 '18 23:11 irobertson