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

[Coleen Phillimore]

On Fri, 5 Jul 2024 14:15:33 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
>
> Patricio Chilano Mateo has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Coleen's comments
>  - David's comments

One tiny nit.

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

> 91:  protected:
> 92: #ifdef ASSERT
> 93:   bool           _used;

Can you make this a DEBUG_ONLY() too?

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

PR Review: https://git.openjdk.org/jdk/pull/20012#pullrequestreview-2160851216
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1666882419