Re: ClassLoader leak in MethodHandle.asType()
On 04/25/2015 02:12 AM, John Rose wrote: On Apr 24, 2015, at 3:24 PM, Peter Levart peter.lev...@gmail.com mailto:peter.lev...@gmail.com wrote: Anyway. The inexact invoke() always transforms a specific MH to a generic one (Object, Object, ...)Object, right? Yes. So using inexact invoke() on any MH can't trigger the leak. It's just that if someone attempts to transform a generic MH to some more concrete one to be able to connect it with further transformations does the danger of the leak appear. Good point, thanks. This suggests that the right answer is to keep the 1-element cache as-is, but don't fill it with any mh1=mh0.asType(...) for which mh1.type introduces a class loader not already present in mh0.type. I.e., don't use a weak reference for the 1-element cache, but don't fill it with anything that might require a weak reference. Use a backup cache (if needed) to handle exotic type changes. So maybe, just for the purpose of inexact invoke(), the caching could be performed just in case when asType transformed method type is of the form (Object, Object, ...)Object. This would also prevent accidental thrashing of the cache when inexact invoke() is intermixed with other asType() invocations. The 1-element cache thrashes for other reasons already, and I think we need a multi-way (multi-MH) backup cache of some sort. The backup cache would have weak references. Having a 1-element front-side cache would cover inefficiencies in the backup cache, which in turn would cover inefficiencies for generating new conversion LFs and/or MHs. One interesting point: If we change the 1-element cache to use a weak reference, the cost of reloading the cache will be higher, since each reload must create a new WeakReference instance. This is a possible reason to hold off on the suggested fix. — John I thought so, yes. But checking for the cache-ability might not be free either. OTOH asTypeUncached() logic is not trivial as it is. It already includes checks like isConvertibleTo, etc, so another check might not be costly relatively. And not introducing another de-reference (WeakReference) makes the fast-path (the cache hit) unaffected. Something like the following then? http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/webrev.02/ Regards, Peter ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: ClassLoader leak in MethodHandle.asType()
On 04/25/2015 12:24 AM, Peter Levart wrote: On 04/24/2015 11:06 PM, John Rose wrote: Good point. Are you seeing a leak in practice? The cache is important, especially to inexact MH.invoke. — John Well, yes. I am (re)implementing annotations (proxies) using Remi's Proxy2 and made it all the way except for the following failing jtreg test: jdk/test/java/lang/annotation/loaderLeak/Main.java ...which made me find this issue. What I'm doing is the following: For implementing some annotation @Ann proxy's hashCode() method for example, I prepare DMHs with the following signature (T)int for Ts being all primitives, primitive arrays, Object[] and Object. I cache them in a system class static field referenced HashMap keyed by ClassT. These are the functions that take an annotation member value and map it to it's hashCode. When the value is of some Object (or Object[] in case it is an array) subtype - for example when the member value is an annotation of type V, I take the (Object)int DMH and transform it with .asType(methodType(int.class, V.class)) to get (V)int, which is necessary to further wrap it with filterArguments where the filter is a member field getter of type (Ann)V so that I get (Ann)int MHs as a kind of hashCodeExtractors for all annotation members (I'll present the details later)... I'm curious to see your implementation. rgds, Rémi ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: ClassLoader leak in MethodHandle.asType()
On Apr 24, 2015, at 3:24 PM, Peter Levart peter.lev...@gmail.com wrote: Anyway. The inexact invoke() always transforms a specific MH to a generic one (Object, Object, ...)Object, right? Yes. So using inexact invoke() on any MH can't trigger the leak. It's just that if someone attempts to transform a generic MH to some more concrete one to be able to connect it with further transformations does the danger of the leak appear. Good point, thanks. This suggests that the right answer is to keep the 1-element cache as-is, but don't fill it with any mh1=mh0.asType(...) for which mh1.type introduces a class loader not already present in mh0.type. I.e., don't use a weak reference for the 1-element cache, but don't fill it with anything that might require a weak reference. Use a backup cache (if needed) to handle exotic type changes. So maybe, just for the purpose of inexact invoke(), the caching could be performed just in case when asType transformed method type is of the form (Object, Object, ...)Object. This would also prevent accidental thrashing of the cache when inexact invoke() is intermixed with other asType() invocations. The 1-element cache thrashes for other reasons already, and I think we need a multi-way (multi-MH) backup cache of some sort. The backup cache would have weak references. Having a 1-element front-side cache would cover inefficiencies in the backup cache, which in turn would cover inefficiencies for generating new conversion LFs and/or MHs. One interesting point: If we change the 1-element cache to use a weak reference, the cost of reloading the cache will be higher, since each reload must create a new WeakReference instance. This is a possible reason to hold off on the suggested fix. — John___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: ClassLoader leak in MethodHandle.asType()
On 04/24/2015 11:06 PM, John Rose wrote: Good point. Are you seeing a leak in practice? The cache is important, especially to inexact MH.invoke. — John Well, yes. I am (re)implementing annotations (proxies) using Remi's Proxy2 and made it all the way except for the following failing jtreg test: jdk/test/java/lang/annotation/loaderLeak/Main.java ...which made me find this issue. What I'm doing is the following: For implementing some annotation @Ann proxy's hashCode() method for example, I prepare DMHs with the following signature (T)int for Ts being all primitives, primitive arrays, Object[] and Object. I cache them in a system class static field referenced HashMap keyed by ClassT. These are the functions that take an annotation member value and map it to it's hashCode. When the value is of some Object (or Object[] in case it is an array) subtype - for example when the member value is an annotation of type V, I take the (Object)int DMH and transform it with .asType(methodType(int.class, V.class)) to get (V)int, which is necessary to further wrap it with filterArguments where the filter is a member field getter of type (Ann)V so that I get (Ann)int MHs as a kind of hashCodeExtractors for all annotation members (I'll present the details later)... Now if V is an annotation type that is loaded by a child class loader, it gets retained by system cached DMH. But now that I'm writing this, I see a workarround. Instead of transforming hashCode function (Object)int - (V)int and applying field getter (Ann)V as an argument filter, I could transform the field getter argument filter (Ann)V - (Ann)Object and apply it directly to the argument of hashCode function (Object)int. Anyway. The inexact invoke() always transforms a specific MH to a generic one (Object, Object, ...)Object, right? So using inexact invoke() on any MH can't trigger the leak. It's just that if someone attempts to transform a generic MH to some more concrete one to be able to connect it with further transformations does the danger of the leak appear. So maybe, just for the purpose of inexact invoke(), the caching could be performed just in case when asType transformed method type is of the form (Object, Object, ...)Object. This would also prevent accidental thrashing of the cache when inexact invoke() is intermixed with other asType() invocations. Regards, Peter On Apr 24, 2015, at 9:14 AM, Peter Levart peter.lev...@gmail.com mailto:peter.lev...@gmail.com wrote: I think there is a Class(Loader) leak in MethodHandle.asType() implementation. If for example some MH (say mhX) is long lived (because it is a system cached MH) and a client generates variants from it by invoking mhX.asType(newType) and the newType contains a type (either return type or parameter type) that is loaded by some child ClassLoader, then such derived MH is retained strongly from mhX via the MethodHandle.asTypeCache field. Until this field is overwriten by some other MH, child ClassLoader can not be GCed. In case this one-element cache is needed to speed things up, it should be a WeakReference, like the following: http://cr.openjdk.java.net/~plevart/jdk9-dev/MethodHandle.asTypeCacheLeak/webrev.01/ http://cr.openjdk.java.net/%7Eplevart/jdk9-dev/MethodHandle.asTypeCacheLeak/webrev.01/ ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
