JNIMethodBlockNode leak question

[Thomas Stüfe]

.. moving to serviceability - maybe you can help me.

Thanks! Thomas


---------- Forwarded message ---------
From: Thomas StÃŒfe <***@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

[JC Beyler]

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.

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.
[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".

Is there a real case where this leak is becoming noticeable?

Thanks,
Jc

Post by Thomas Stüfe
.. moving to serviceability - maybe you can help me.
Thanks! Thomas
---------- Forwarded message ---------
Date: Wed, Nov 7, 2018, 11:38
Subject: JNIMethodBlockNode leak question
Hi all,
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

[Thomas Stüfe]

Hi JC,

Post by JC Beyler
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.

Post by JC Beyler
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.

Post by JC Beyler
[1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#SetBreakpoint
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.

Post by JC Beyler
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...

Post by JC Beyler
Thanks,
Jc

Thanks for looking!

Best Regards, Thomas

Post by JC Beyler

Post by Thomas Stüfe
.. moving to serviceability - maybe you can help me.
Thanks! Thomas
---------- Forwarded message ---------
Date: Wed, Nov 7, 2018, 11:38
Subject: JNIMethodBlockNode leak question
Hi all,
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

[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

Post by Thomas Stüfe
Hi JC,

Post by JC Beyler
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.

Post by JC Beyler
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.

Post by JC Beyler
[1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#SetBreakpoint
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.

Post by JC Beyler
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...

Post by JC Beyler
Thanks,
Jc

Thanks for looking!
Best Regards, Thomas

Post by JC Beyler

Post by Thomas Stüfe
.. moving to serviceability - maybe you can help me.
Thanks! Thomas
---------- Forwarded message ---------
Date: Wed, Nov 7, 2018, 11:38
Subject: JNIMethodBlockNode leak question
Hi all,
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

[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

Post by JC Beyler
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

Post by Thomas Stüfe
Hi JC,

Post by JC Beyler
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.

Post by JC Beyler
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.

Post by JC Beyler
[1] https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#SetBreakpoint
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.

Post by JC Beyler
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...

Post by JC Beyler
Thanks,
Jc

Thanks for looking!
Best Regards, Thomas

Post by JC Beyler

Post by Thomas Stüfe
.. moving to serviceability - maybe you can help me.
Thanks! Thomas
---------- Forwarded message ---------
Date: Wed, Nov 7, 2018, 11:38
Subject: JNIMethodBlockNode leak question
Hi all,
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