On Mon, 7 Oct 2024 08:15:21 GMT, Erik Österlund <eosterl...@openjdk.org> wrote:
>> Aleksey Shipilev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> More precise bit-unmasks
>
> src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 342:
>
>> 340: }
>> 341: if ((on_weak || on_phantom) && no_keepalive) {
>> 342: // Be extra paranoid around this path. Only accept null stores,
>
> I think there might be some orthogonal stuff that is unnecessarily mixed up
> here. When no_keepalive is manually specified, then we shouldn't do the
> pre-write barrier, regardless of reference strength. Similarly, when the new
> value is null, we don't need to perform the post write barrier, regardless of
> reference strength. Roberto added some code in refine_barrier_by_new_val_type
> that already *should* take care of the latter part. It allows types to flow
> around a bit, and then checks if the type of the new value is provably null,
> and then removes the post write barrier. The existing logic for that should
> be strictly more powerful than the new check you added, I think.
>
> Based on the above explanation, I think I'm proposing this block is replaced
> with this simpler condition:
>
> if (no_keepalive) {
> access.set_barrier_data(access.barrier_data() & ~G1C2BarrierPre);
> }
Right. We also do not need this complexity in Shenandoah barriers. This check
was dragged here from the load barriers that _want_ to check if we are reading
the `Reference.referent` and feed it to SATB _unless_ there is a no-keep-alive.
For store barriers it is unnecessary, and we can just do keep-alive checks
straight up. Should be done in new commit, testing now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20139#discussion_r1793115683
