On Tue, 8 Oct 2024 08:12:31 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> 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.
>
> I was thinking that we should remove the entire:
>
> // Note: If min-fill-size decreases to 1, this whole method becomes
> redundant.
> assert(CollectedHeap::min_fill_size() >= 2, "inv");
>
> block, since it is now incorrect, guarded by the proper check, and the
> comment is misleading since we now can have a min-fill-size that is 1.
It's still correct when UCOH is disabled -- therefore, the UCOH check can be
placed at the start without changing any existing logic. (The "rest" of this
method assumes min-fill-size is 2, `assert(CollectedHeap::min_fill_size() == 2,
"inv")`.)
In this PR, since this method doesn't access UCOH, it can be easily forgotten
to update this method when the UCOH flag is removed eventually -- it's not
obvious to me that `MinObjAlignment >=
checked_cast<int>(CollectedHeap::min_fill_size())` is related to (or can be
affected by) `UCOH` at first glance.
(I slightly prefer having a `if (UCOH)` inside this method, but considering
this method will be nuked in the long run, any short-time decision is fine by
me, assuming the failing assert is fixed.)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1791611304
