RFR: 8334060: Implementation of Late Barrier Expansion for G1 [v27]

[Roberto Castañeda Lozano]

On Thu, 3 Oct 2024 17:12:04 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Roberto Castañeda Lozano has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 53 additional commits since the last revision:
>> 
>>  - Merge remote-tracking branch 'feilongjiang/JEP-475-RISC-V' into JDK-8334060-g1-late-barrier-expansion
>>  - riscv port refactor
>>  - Remove temporary support code
>>  - Merge jdk-24+17
>>  - Relax 'must match' assertion in ppc's g1StoreN after limiting pinning bypass optimization
>>  - Remove unnecessary reg-to-reg moves in aarch64's g1CompareAndX instructions
>>  - Reintroduce matcher assert and limit pinning bypass optimization to non-shared EncodeP nodes
>>  - Merge jdk-24+16
>>  - Ensure that detected encode-and-store patterns are matched
>>  - Merge remote-tracking branch 'snazarkin/arm32-JDK-8334060-g1-late-barrier-expansion' into JDK-8334060-g1-late-barrier-expansion
>>  - ... and 43 more: https://git.openjdk.org/jdk/compare/0165cb32...14483b83
>
> src/hotspot/share/gc/g1/c2/g1BarrierSetC2.cpp line 335:
> 
>> 333:       assert(!use_ReduceInitialCardMarks(),
>> 334:              "post-barriers are only needed for tightly-coupled initialization stores when ReduceInitialCardMarks is disabled");
>> 335:       access.set_barrier_data(access.barrier_data() ^ G1C2BarrierPre);
> 
> I have been looking at this code after integration, and I wonder if `^` is really correct here? Was the intend to remove `G1C2BarrierPre` from the barrier data? What happens if `get_store_barrier` does not actually set it? Do we flip the bit back?

Yes, the intend (and actual effect) is to remove `G1C2BarrierPre` from the barrier data. Using an XOR (`^`) is correct because at that program point `G1C2BarrierPre` is guaranteed to be set. This is because an `access` corresponding to a tightly-coupled initialization store is always of type `C2OptAccess`, hence `!access.is_parse_access()` and `get_store_barrier(access)` trivially returns `G1C2BarrierPre | G1C2BarrierPost`. Having said this, it would be clearly less convoluted to simply clear `G1C2BarrierPre` instead of flipping it. I will file a RFE, thanks.

As a side note, this complexity is necessary to handle `!ReduceInitialCardMarks`. I keep wondering if the benefit of being able to disable `ReduceInitialCardMarks` [1,2,3] is worth the significant complexity required in the GC-C2 interface to deal with it.

[1] https://docs.oracle.com/en/java/javase/23/gctuning/garbage-first-garbage-collector-tuning.html
[2] https://bugs.openjdk.org/browse/JDK-8166899
[3] https://bugs.openjdk.org/browse/JDK-8167077

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19746#discussion_r1787425169