RFR: 8332527: ZGC: generalize object cloning logic [v2]

[Roberto Castañeda Lozano]

On Tue, 21 May 2024 06:11:17 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

> lgtm.
> 
> Feel free to use, change or discard my suggestions.

Thanks for reviewing Axel! I just applied your 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");

You are right, using just "instances" is more idiomatic, changed.

> 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);

No good reason, just follows from the pre-existing code, I changed it now.

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

PR Comment: https://git.openjdk.org/jdk/pull/19311#issuecomment-2122156087
PR Review Comment: https://git.openjdk.org/jdk/pull/19311#discussion_r1607942604
PR Review Comment: https://git.openjdk.org/jdk/pull/19311#discussion_r1607944721