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

[Patricio Chilano Mateo]

On Mon, 8 Jul 2024 12:19:48 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:
>> 
>>   use DEBUG_ONLY on _used declaration
>
> 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.

Fixed. We could had such case if the mask was never filled due to invalid bci, so I also improved the conditional.

> 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

Fixed, should be fine to do it in this PR.

> 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.

Right, removed.

> 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?

We need them here otherwise we get a compilation error on the conversion from intptr_t to void*. But we don't need them above so I removed those.

> 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?

Yes, fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1669071718
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1669072023
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1669073705
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1669073099
PR Review Comment: https://git.openjdk.org/jdk/pull/20012#discussion_r1669072432