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

[Patricio Chilano Mateo]

On Thu, 27 Jun 2024 13:56:15 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   take ResourceMark out of debug only
>
> 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.

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

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