Fwd: JNIMethodBlockNode leak question

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Fwd: JNIMethodBlockNode leak question

Thomas Stüfe-2
.. moving to serviceability - maybe you can help me.

Thanks! Thomas 


---------- Forwarded message ---------
From: Thomas Stüfe <[hidden email]>
Date: Wed, Nov 7, 2018, 11:38
Subject: JNIMethodBlockNode leak question
To: Hotspot dev runtime <[hidden email]>


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
Reply | Threaded
Open this post in threaded view
|

Re: JNIMethodBlockNode leak question

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.

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



On Thu, Nov 8, 2018 at 12:02 AM Thomas Stüfe <[hidden email]> wrote:
.. moving to serviceability - maybe you can help me.

Thanks! Thomas 


---------- Forwarded message ---------
From: Thomas Stüfe <[hidden email]>
Date: Wed, Nov 7, 2018, 11:38
Subject: JNIMethodBlockNode leak question
To: Hotspot dev runtime <[hidden email]>


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
Reply | Threaded
Open this post in threaded view
|

Re: JNIMethodBlockNode leak question

Thomas Stüfe-2
Hi JC,

On Thu, Nov 8, 2018 at 5:49 PM JC Beyler <[hidden email]> 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 <[hidden email]> wrote:
>>
>> .. moving to serviceability - maybe you can help me.
>>
>> Thanks! Thomas
>>
>>
>> ---------- Forwarded message ---------
>> From: Thomas Stüfe <[hidden email]>
>> Date: Wed, Nov 7, 2018, 11:38
>> Subject: JNIMethodBlockNode leak question
>> To: Hotspot dev runtime <[hidden email]>
>>
>>
>> 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
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> 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 <[hidden email]> wrote:
>>>
>>> .. moving to serviceability - maybe you can help me.
>>>
>>> Thanks! Thomas
>>>
>>>
>>> ---------- Forwarded message ---------
>>> From: Thomas Stüfe <[hidden email]>
>>> Date: Wed, Nov 7, 2018, 11:38
>>> Subject: JNIMethodBlockNode leak question
>>> To: Hotspot dev runtime <[hidden email]>
>>>
>>>
>>> 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
Reply | Threaded
Open this post in threaded view
|

Re: JNIMethodBlockNode leak question

Thomas Stüfe-2
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
> >>>
> >>> .. moving to serviceability - maybe you can help me.
> >>>
> >>> Thanks! Thomas
> >>>
> >>>
> >>> ---------- Forwarded message ---------
> >>> From: Thomas Stüfe <[hidden email]>
> >>> Date: Wed, Nov 7, 2018, 11:38
> >>> Subject: JNIMethodBlockNode leak question
> >>> To: Hotspot dev runtime <[hidden email]>
> >>>
> >>>
> >>> 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