On Mon, 7 Oct 2024 11:49:21 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> When MinObjectAlignment=16, then that method does nothing anyway:
>>
>>
>> if (MinObjAlignment > 1) {
>> return;
>> }
>>
>>
>>
>> I think what it really means to say is
>>
>> if (MinObjAlignment >= CollectedHeap::min_fill_size()) {
>> return;
>> }
>>
>>
>>
>> That's also what the comment says: "The size of the gap (if any) right
>> before dense-prefix-end is MinObjAlignment. Need to fill in the gap only if
>> it's smaller than min-obj-size, and the filler obj will extend to next
>> region."
>>
>> If I interpret that correctly, we need to deal with the situation only when
>> MinObjAlignment < min_fill_size, because the filler object would extend to
>> the next region, and we need to adjust the next region and mark-bitmap for
>> that extra word. @albertnetymk might want to confirm.
>>
>> I'll move the if (UCOH) block down a little bit to right before if
>> (MinObjAlignment) block.
>
> After re-reading this again I agree with what you're writing. If you make the
> change to use:
>
> if (MinObjAlignment >= CollectedHeap::min_fill_size()) {
> return;
> }
>
>
> do you even have to check for UCOH in this function?
>
> I also wonder if you could tweak the comment now that this is not true when
> UCOH is on:
>
> // The size of the filler (min-obj-size) is 2 heap words with the default
> // MinObjAlignment, since both markword and klass take 1 heap word.
I took UCOH into account when this code was written -- the current version of
PR would fail the following assert.
// Note: If min-fill-size decreases to 1, this whole method becomes redundant.
assert(CollectedHeap::min_fill_size() >= 2, "inv");
The least intrusive way, IMO, is to put `if (UCOH) { return; }` right before
`// Note: ...`, kind of like what Roman originally put it. I believe the
advantage of this style is that when UCOH before always-true, it's obvious this
whole method essentially becomes `return`and can be removed right away.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1791362310
