7127687: MethodType leaks memory due to interning

[Rémi Forax]

On 03/29/2012 08:45 AM, Bengt Rutisson wrote:
>
> Hi John,
>
> This is really not a part of the code that I know well. I don't think 
> I can fully review this change, but I do have some questions.
>
> First, I have the same concern as David. This change means that we 
> will produce more WeakReference objects. Since I don't know the code 
> that well I can't estimate if it will be many or few. But it seems to 
> me that if you get a PermGen OOME now you are probably creating quite 
> a few instances of the MethodType that you will add to the 
> WeakInternSet, right? I think we can expect some performance issues in 
> the GC from that.
>
> Second, I still don't really understand how WeakInternSet is different 
> from what Vitaly suggested. To just use WeakHashMap with a dummy 
> object as value. Can you explain the difference in more detail?
>
> The CR mentions another way to solve the issue: "We could intern 
> MethodTypes involving only bootstrap types. For other MethodTypes, we 
> could always use .equals to compare rather than identity comparison."
>
> Again, I really don't know what I am talking about here, but would 
> that solution avoid the WeakReferences? If yes, would it be possible 
> to pick that solution instead?

yes.

Hotspot requires two MethodType that are type of a Methodhandle to be ==
in order to speed up the implementation of MethodHandle.invokeExact (and 
invoke).
The JSR 292 spec doesn't require MethodType to be interned so all 
MethodTypes
that are not used as type of a MethodHandle doesn't have to be interned.
The interning of the MethodType can be done only for method type that 
are stored
in the field named type of a MethodHandle.

I don't think it's good idea because this will create more MethodType, 
some non interned created by the user
and some interned created from the ones created by the user.
By example, currently a Lookup.findStatic takes a MethodType as 
parameter and this instance is
directly used as type of the resulting MethodHandle. In that case, you 
have just added a new allocation
to the implementation because the MethodType will have to be interned 
anyway.

This is not always the case because the implementation use transient 
MethodType
but I think it's better to change the implementation to avoid to use 
these transient MethodType
even if it will make the code less clear.

Another solution is to intern the MethodType the first time a 
MethodHandle is called
but in that case the type of a MethodHandle is not constant anymore and
this may create serious concurrency issues.

>
>
> Two minor things as well:
>
> * Don't think you need "import java.util.HashMap;" anymore.
> * Copyright year 2012 ;-)
>
> Bengt

Rémi