JNIMethodBlockNode leak question

[Thomas Stüfe]

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