RFR: 8139457: Array bases are aligned at HeapWord granularity [v11]

[Stefan Karlsson]

On Thu, 19 Jan 2023 12:17:15 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 4136:
>> 
>>> 4134:   assert(is_aligned(offset_in_bytes, BytesPerInt), "offset must be a multiple of BytesPerInt");
>>> 4135: 
>>> 4136:   // note: for the remaining code to work, index must be a multiple of BytesPerInt
>> 
>> Maybe move the comment to above the assert that checks the invariant described by the comment?
>
> I think it is. The whole block below the comment is the runtime check for the described invariant. I re-worded it to be a bit clearer:
> // For the remaining code to work, length must be a multiple of BytesPerInt.
> // Check that here.

OK. Makes sense.

>> src/hotspot/share/gc/shared/collectedHeap.cpp line 444:
>> 
>>> 442:   payload_start = payload_start / HeapWordSize;
>>> 443:   Copy::fill_to_words(start + payload_start,
>>> 444:                       words - payload_start, value);
>> 
>> This mixes bytes and words. I don't think this is correct.
>
> How does it mix bytes and words? start is a heapword*, payload_start is in word units (I am going to change the line above to make it clearer, words is words ;-).

Oh, I see now, you reassigned the variable that used to contain a value in bytes to contain a value in words. How evil. I'd prefer if never do things like that in HotSpot. Could this be split into two variables?

>> src/hotspot/share/gc/shared/memAllocator.cpp line 427:
>> 
>>> 425:   }
>>> 426:   ArrayKlass* array_klass = ArrayKlass::cast(_klass);
>>> 427:   const size_t hs = heap_word_size(arrayOopDesc::base_offset_in_bytes(array_klass->element_type()));
>> 
>> I don't think this makes sense. If base_offset_in_bytes return something that isn't word aligned, then `heap_word_size` will truncate the value and `hs` will less than the actual header size. This then snowballs back to the caller of `obj_memory_range`. Maybe we need change obj_memory_range to work with bytes instead of words?
>
> I think that heap_word_size() aligns up:
> inline size_t heap_word_size(size_t byte_size) {
>   return (byte_size + (HeapWordSize-1)) >> LogHeapWordSize;
> }
> which makes obj_memory_range() ok as it is? I agree that it is not great, but it seems to be used only by check_for_bad_heap_word_value(), and for that purpose it is ok? Not 100% sure.

You are right that it aligns up. I agree that it's not 100% clear that check_for_bad_heap_word_value is correct.

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

PR: https://git.openjdk.org/jdk/pull/11044