RFR: 8329665: fatal error: memory leak: allocating without ResourceMark [v2]

[Thomas Stuefe]

On Mon, 1 Jul 2024 12:42:44 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> I am a bit concerned about this fix. Introducing an RM into `frame::oops_interpreted_do` means we cannot assemble anything in RA in the closure code and keep the memory across the RM. But closure code is opaque to the iteration site. Do we have any safeguards against OopClosure using and retaining RA memory? (Because even if no closure does this today, this could sneak in easily)
>
> @tstuefe I added checks and I found there is one case in JFR code where we can allocate and retain memory from the resource area across the closure (https://github.com/openjdk/jdk/blob/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleWriter.cpp#L462). It can be triggered by running tests in jdk/jfr/event/oldobject.
> 
> There are a couple of options I can think of here:
> 
> 1 - Fix this case and add a safeguard to prevent this allocations. Maybe have some ResetRM object before mask.iterate_oop to reset the nesting in the RA on construction and then restore it on destruction.
> 2 - Allocate the _bit_mask in the C heap instead. There is a comment in InterpreterOopMap::resource_copy() that this has a significant performance cost. But we are already allocating an OopMapCacheEntry from the C heap, and this allocation for the bit_mask should be a rare case so I doubt this.
> 3 - Have the callers of frame::oops_interpreted_do() declare the ResourceMark instead. I wanted to avoid this because this is an implementation detail of InterpreterOopMap.
> 4 - Restore the code as it was before this change and add an object before NEW_RESOURCE_ARRAY(...) in InterpreterOopMap::resource_copy() that increments the nesting in the RA on construction and decrements it on destruction, i.e basically mark this particular allocation as okay without a RM since we check for it in ~InterpreterOopMap.
> 
> What do you think? I don't actually see a particular reason to disallow this allocations so I'm now not convinced on 1.

Hi @pchilano 

> @tstuefe I added checks and I found there is one case in JFR code where we can allocate and retain memory from the resource area across the closure (
> 
> https://github.com/openjdk/jdk/blob/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d/src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleWriter.cpp#L462
> ). It can be triggered by running tests in jdk/jfr/event/oldobject.
> 

Yes, I was afraid of something like that. Please open a follow-up bug, and chain it to 8329665, since the fix has already been downported to JDK21. We should fix this before the October update. I added a little comment to 8329665 to ensure this does not get lost.

> There are a couple of options I can think of here:
> 
> 1 - Fix this case and add a safeguard to prevent this allocations. Maybe have some ResetRM object before mask.iterate_oop to reset the nesting in the RA on construction and then restore it on destruction. 

Safeguarding is not so easy. You could observe the RA state at the end and compare it with the beginning, but the code may use RM in a benign way. 

> 2 - Allocate the _bit_mask in the C heap instead. There is a comment in InterpreterOopMap::resource_copy() that this has a significant performance cost. But we are already allocating an OopMapCacheEntry from the C heap, and this allocation for the bit_mask should be a rare case so I doubt this.

I was thinking along of this line.

The performance comment makes sense for lookup, but not so much here. Plus, I would wager this case is rare anyway since the InterpreterOopMap can hold what, (4*64)/2bit = 128 entries?

About the other comment (` // Due to the invariants above it's tricky to allocate a temporary OopMapCacheEntry on the stack`), that is weird. I don't get it. It is ages old (comes from initial commit), so you may be more able than I to dig out the history behind it. I tried to just allocate an InterpreterOopMap entry on the stack. I did a small test, nothing bad happened.

So, if this comment is not up to date, you may pay for the C-heap allocation by removing this allocation :)

> 3 - Have the callers of frame::oops_interpreted_do() declare the ResourceMark instead. I wanted to avoid this because this is an implementation detail of InterpreterOopMap. 

Yes, I don't like this either.

> 4 - Restore the code as it was before this change and add an object before NEW_RESOURCE_ARRAY(...) in InterpreterOopMap::resource_copy() that increments the nesting in the RA on construction and decrements it on destruction, i.e basically mark this particular allocation as okay without a RM since we check for it in ~InterpreterOopMap.

Not a fan, overly complicated.

> 
> What do you think? I don't actually see a particular reason to disallow this allocations so I'm now not convinced on 1.

I prefer 2.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/18632#issuecomment-2200093475