RFR: 8328602: Parallel: Incorrect assertion in fill_dense_prefix_end

[Albert Mingkun Yang]

On Thu, 21 Mar 2024 13:33:35 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>>> When `MinObjAlignment` is 8 or 16 (`MinObjAlignment` is 1 or 2),
>> 
>> Sorry, one typo. The first `MinObjAlignment` should be `ObjectAlignmentInBytes`. I fixed the previous comment.
>> 
>>> Could these two assert statement be simplified to one?
>> 
>> Actually, the second `assert` can be removed because we can deduce it according to the first `assert` and the `if` statement.
>> 
>> I provided such unclear guide just because I think you or others may have a better way to revise the code (instead of simply removing the second one).
>
> Concretely, when the first `assert` statement is `true`, we can know `MinObjAlignment >= 1`. Then the `if` statement excludes the `MinObjAlignment > 1`. So, only when `MinObjAlignment == 1`, the code can run after the `if` statement, then the `min_fill_size()` must be **2**.
> 
> 
>   assert(CollectedHeap::min_fill_size() >= 2, "inv");
> // <-- here, `MinObjAlignment >= 1`
>   if (MinObjAlignment > 1) {   // exclude the `MinObjAlignment > 1`
>     return;
>   }
> // <-- only one condition: `MinObjAlignment == 1`
>   assert(CollectedHeap::min_fill_size() == 2, "inv"); // <-- This assert must be true and unnecessary, because `MinObjAlignment == 1`.

I see what you mean. The second assert is mostly for documentation purpose. Readers might not connect `min_fill_size` with `MinObjAlignment` right away.

The second assert can probably be removed if the `if-check` is written as `if (CollectedHeap::min_fill_size() > 2)`. I didn't go for that version, as it might introduce more assembly, but maybe the impact/overhead is negligible, if any.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18396#discussion_r1533956936