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

[Thomas Stuefe]

On Fri, 5 Jul 2024 15:01:05 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 one additional commit since the last revision:
> 
>   use DEBUG_ONLY on _used declaration

Small nits, otherwise good. Thanks a lot for fixing.

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

> 181:   if (mask_size() > small_mask_limit) {
> 182:     assert(!Thread::current()->resource_area()->contains((void*)_bit_mask[0]),
> 183:            "The bit mask should be allocated from the C heap");

Arguably, this assert is not needed. In debug builds, we have NMT enabled, and that does a check on os::free.

However, an assert that _bit_mask[0] != 0 *would* make sense, since the free quielty swallows null pointers.

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

> 403: // Implementation of OopMapCache
> 404: 
> 405: void InterpreterOopMap::copy_from(OopMapCacheEntry* src) {

Possibly for another  RFE: src pointer should be const

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

> 421:   } else {
> 422:     _bit_mask[0] = (uintptr_t) NEW_C_HEAP_ARRAY(uintptr_t, mask_word_size(), mtClass);
> 423:     assert(_bit_mask[0] != 0, "bit mask was not allocated");

The assert can be removed, no? NEW_C_HEAP_ARRAY does a null check by default.

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

> 422:     _bit_mask[0] = (uintptr_t) NEW_C_HEAP_ARRAY(uintptr_t, mask_word_size(), mtClass);
> 423:     assert(_bit_mask[0] != 0, "bit mask was not allocated");
> 424:     memcpy((void*) _bit_mask[0], (void*) src->_bit_mask[0], mask_word_size() * BytesPerWord);

Are the (void*) cast really needed?

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

> 90: 
> 91:  protected:
> 92:   DEBUG_ONLY(bool _used;)

Minor nit. This changes memory layout between debug and release builds, and this is used as part of OopMapCache. Not a big concern, but I usually prefer having the same layout between debug and release to test what we ship.

Can't we not just assert that mask size == USHRT_MAX?

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

PR Review: https://git.openjdk.org/jdk/pull/20012#pullrequestreview-2163133516
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1668523910
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1668536249
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1668539154
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1668535458
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1668531734