JNIMethodBlockNode leak question

[David Holmes]

Hi Thomas,

I did finally get a chance to look into this, not that I have anything 
new to add. We'd need a much more sophisticated scheme to allow the old 
"id's" to remain usable but invalid, whilst still allowing the tables to 
be reclaimed.

Cheers,
David

On 9/11/2018 5:44 AM, Thomas Stüfe wrote:
> Hi JC,
> 
> On Thu, Nov 8, 2018 at 5:49 PM JC Beyler <jcbeyler at google.com> wrote:
>>
>> Hi Thomas,
>>
>> When I follow the code flow, yes that seems right; your comment " a caller may hand it in long after the class has been unloaded, and this should be handled gracefully" is exactly what the spec says for example for a lot of the JVMTI methods that take a methodId such as SetBreakpoint[1] for example.
> 
> Oh right. I was struggling to find a good example for why we cannot
> let jmethodids expire. But sure, the debugger could have set there for
> an hour after calling GetClassMethods, and the jmethodids could be all
> stale.
> 
>>
>> Doing it this way is the easiest (safest?) way to ensure that if a class is unloaded, there is a means to be passing invalid method ids to JVMTI methods without having everything break or having to have in memory what methods are still valid.
> 
> Its also quite cheap. We pay 8 bytes of leaked memory per jmethodid,
> plus a bit of overhead. I struggle to come up with a better scheme. I
> guess one could, with some sort of offset based addressing, trim this
> down to 32bit? Or some magic with un-committing the underlying memory
> under the JNIMethodBlockNodes but keeping the address space reserved,
> and then catching the segfault with SafeFetch. But I do not know if it
> would be worth the effort.
> 
>> [1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#SetBreakpoint
>>
>> Or more in detail, a lot of JVMTI methods have a nice:
>>    NULL_CHECK(method_oop, JVMTI_ERROR_INVALID_METHODID);
>>
>> That allows to do this check and, so technically, the comment could be amended to say "JVMTI also is expecting this".
>>
> I agree.
> 
>> Is there a real case where this leak is becoming noticeable?
>>
> 
> Yes, we have this client which uses our VM in weird and intricate
> ways. Embedded in an own launcher, lots of JNI coding. They seem to be
> doing just that: over and over reloading classes in new class loaders
> and accessing them via JNI. Last I saw they allocated 250 MB worth of
> JNIMethodBlockNodes, which would amount to roughly 3 million jmethodid
> handles...
> 
>> Thanks,
>> Jc
>>
> 
> Thanks for looking!
> 
> Best Regards, Thomas
> 
>>
>>
>> On Thu, Nov 8, 2018 at 12:02 AM Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
>>>
>>> .. moving to serviceability - maybe you can help me.
>>>
>>> Thanks! Thomas
>>>
>>>
>>> ---------- Forwarded message ---------
>>> From: Thomas Stüfe <thomas.stuefe at gmail.com>
>>> Date: Wed, Nov 7, 2018, 11:38
>>> Subject: JNIMethodBlockNode leak question
>>> To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
>>>
>>>
>>> Hi all,
>>>
>>> I wonder whether I understand this correctly:
>>>
>>> When a caller requests a jmethodId bei either calling
>>> jni-GetMethodId() or jvmti-GetClassMethods(), the returned jmethodId
>>> is a pointer into a slot of a table containing the respective Method*
>>> pointers. That table is tied to the ClassLoaderData. When the
>>> classloader is unloaded and its ClassLoaderData deleted, we do not
>>> delete that table, but rather deliberately leak it after zeroing it
>>> out. The reason - if I understand the comment in ~ClassLoaderData
>>> correctly - is that jmethodId live forever - a caller may hand it in
>>> long after the class has been unloaded, and this should be handled
>>> gracefully.
>>>
>>> We cannot simply delete its JNIMethodBlockNode, since the jmethodId
>>> then would either point to unmapped memory - which we could handle
>>> with SafeFetch - but worse could point to memory reused for a
>>> different purpose.
>>>
>>> We also could not reuse that slot, since then that jmethodId would
>>> point to a different method? Apart from the fact that we would need
>>> infrastructure for that, a global pool of JNIMethodBlockNode, with
>>> associated locking etc.
>>>
>>> Have I understood this correctly or am I off somewhere?
>>>
>>> And if I am right, that would mean that were we to generate classes
>>> over and over again in new class loaders and accessing their methods
>>> with JNI, we would have a slowly growing leak, even if we clean up the
>>> loaders?
>>>
>>> Thanks, Thomas
>>
>>
>>
>> --
>>
>> Thanks,
>> Jc