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

[Coleen Phillimore]

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

Also a couple of nits, but this looks good.  Thanks for tracking down the history and verifying that its an unusual situation that we were optimizing for.

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

> 43: // For InterpreterOopMap the bit_mask is allocated in the C heap
> 44: // to avoid issues with allocations from the resource area that have
> 45: // to live accross the oop closure (see 8335409). InterpreterOopMap

We don't usually put bug numbers in the code and after this change nobody will want to move this back to resource area, so putting the bug number as a caution shouldn't be needed.  If one wants to know the details, they can git blame this file.

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

> 44: // to avoid issues with allocations from the resource area that have
> 45: // to live accross the oop closure (see 8335409). InterpreterOopMap
> 46: // should only be created and deleted during same garbage collection.

Can you add 'the' to "during the same garbage collection."

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

PR Review: https://git.openjdk.org/jdk/pull/20012#pullrequestreview-2160631864
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1666753678
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1666754397