RFR: 8335409: Can't allocate and retain memory from resource area in frame::oops_interpreted_do oop closure after 8329665

[David Holmes]

On Wed, 3 Jul 2024 16:24:20 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> The ResourceMark added in 8329665 to address the case of having to allocate extra memory for the _bit_mask, prevents code in the closure from allocating and retaining memory from the resource area across the closure, relying on some ResourceMark in scope further up the stack from frame::oops_interpreted_do(). There is in fact one case today in JFR code where this kind of allocation happens.
> 
> The amount of locals and expression stack entries a method can have before having to allocate extra memory for the _bit_mask is 4*64/2 = 128. This is already big enough that we almost never have to allocate. A test run through mach5 tiers1-6 shows only a handful of methods that fall into this case, and most are artificial ones created to trigger this condition. So moving the allocation to the C heap shouldn't have any performance penalty as the comment otherwise says. This comment dates back from 2002 where instead of 128 entries we could have only 32, considering 32 bits cpus as still in main use (see bug for more history details).
> 
> The current code in InterpreterOopMap::resource_copy() has a comment expecting the InterpreterOopMap object to be recently created and empty, but it also has an assert in the allocation case path where it considers the entry might be in use already. This assert actually looks wrong since a used InterpreterOopMap object will not necessarily contain a pointer to resource area memory in _bit_mask[0]. I added an example case in the bug details. In any case, since we don't have any such cases in the codebase I added an explicit assert to verify each InterpreterOopMap is only used one. 
> 
> I tested the patch by running it through mach5 tiers 1-6.
> 
> Thanks,
> Patricio

Thanks for the detailed explanations. A couple of minor (pre-existing) nits but changes are good.

Thanks

src/hotspot/share/interpreter/oopMapCache.cpp line 179:

> 177: #ifdef ASSERT
> 178:   _used = false;
> 179: #endif

Nit pre-existing: use of DEBUG_ONLY would be more consistent with later setting of `_used`.

src/hotspot/share/interpreter/oopMapCache.cpp line 408:

> 406: 
> 407: void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) {
> 408:   // The expectation is that this InterpreterOopMap is a recently created

s/is a recently/is recently/

src/hotspot/share/interpreter/oopMapCache.hpp line 136:

> 134:   // Copy the OopMapCacheEntry in parameter "from" into this
> 135:   // InterpreterOopMap.  If the _bit_mask[0] in "from" points to
> 136:   // allocated space (i.e., the bit mask was to large to hold

Nit pre-existing: s/to/too/

src/hotspot/share/interpreter/oopMapCache.hpp line 138:

> 136:   // allocated space (i.e., the bit mask was to large to hold
> 137:   // in-line), allocate the space from the C heap.
> 138:   void resource_copy(OopMapCacheEntry* from);

The name `resource_copy` seems somewhat of a misnomer given it may be C heap. Is it worth changing?

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20012#pullrequestreview-2157946377
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1665114778
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1665112766
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1665116975
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1665117670