JNIMethodBlockNode leak question

[Thomas Stüfe]

Thank you David.

I agree, fixing or mitigating this leak would require a bit of
thinking. Not sure if it is worth it.

Best Regards, Thomas
On Mon, Nov 12, 2018 at 1:15 AM David Holmes <david.holmes at oracle.com> wrote:
>
> 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