RFR: 8332527: ZGC: generalize object cloning logic

[Axel Boldt-Christmas]

On Mon, 20 May 2024 14:31:26 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

> This changeset generalize the logic to produce a runtime call to clone a class instance so that it can be shared by other collectors adopting the late barrier expansion model (including G1 in the near future, see [JEP 475](https://openjdk.org/jeps/475)). The changeset moves the logic from `ZBarrierSetC2` to the GC-shared `BarrierSetC2` class and adds support for 32-bits platforms.
> 
> #### Testing
> 
> - tier1-3 (windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64; release and debug mode).
> - tier4-7 (linux-x64, linux-aarch64; release and debug mode; ZGC tests only).
> - `compiler/arraycopy` tests (linux-x86-debug) with [an additional patch](https://github.com/openjdk/jdk/commit/ddcf777894e740b8e6ddbbf8821e82a173c23ef4) that implements cloning of large class instances with a runtime clone call rather than arraycopy when using G1 (to exercise the generalized logic on a 32-bits platform).

lgtm.

Feel free to use, change or discard my suggestions.

src/hotspot/share/gc/shared/c2/barrierSetC2.cpp line 840:

> 838: void BarrierSetC2::clone_instance_in_runtime(PhaseMacroExpand* phase, ArrayCopyNode* ac,
> 839:                                              address clone_addr, const char* clone_name) const {
> 840:   assert(ac->is_clone_inst(), "this function is only defined for cloning class instances");

Saying `class instances` is confusing to me. This is used for all instance objects, not only instances of Class objects. Maybe this is some terminology I am unfamiliar with, but in general hotspot uses instance vs array to distinguish between the two classes of objects. E.g. `instanceOop vs arrayOop`, `InstanceKlass vs ArrayKlass`. 
Suggestion:

  assert(ac->is_clone_inst(), "this function is only defined for cloning instances");

src/hotspot/share/gc/shared/c2/barrierSetC2.cpp line 852:

> 850:   // The native clone we are calling here expects the instance size in words.
> 851:   // Add header/offset size to payload size to get instance size.
> 852:   Node* const base_offset = phase->MakeConX(arraycopy_payload_base_offset(ac->is_clone_array()) >> LogBytesPerLong);

Why query for `is_clone_array()` when it is known false from the context we are in? (I know it is what the previous code did, but I am curious why it would be preferred.)
Suggestion:

  Node* const base_offset = phase->MakeConX(arraycopy_payload_base_offset(false /* is_array */) >> LogBytesPerLong);

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

Marked as reviewed by aboldtch (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19311#pullrequestreview-2067611931
PR Review Comment: https://git.openjdk.org/jdk/pull/19311#discussion_r1607690156
PR Review Comment: https://git.openjdk.org/jdk/pull/19311#discussion_r1607690251