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.hol...@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 <jcbey...@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.stu...@gmail.com> > >> wrote: > >>> > >>> .. moving to serviceability - maybe you can help me. > >>> > >>> Thanks! Thomas > >>> > >>> > >>> ---------- Forwarded message --------- > >>> From: Thomas Stüfe <thomas.stu...@gmail.com> > >>> Date: Wed, Nov 7, 2018, 11:38 > >>> Subject: JNIMethodBlockNode leak question > >>> To: Hotspot dev runtime <hotspot-runtime-...@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
