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

[Axel Boldt-Christmas]

On Wed, 22 May 2024 05:21:54 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> Why is `ac->is_clone_array()` known to be always false? And if so, why is there another check at line 433 of `ZBarrierSetC2::clone_at_expansion`?

Good catch.

'ac' in `clone_at_expansion` can be for either. It, as you point out, condition what it does based on if this is a instance clone or an array clone. https://github.com/openjdk/jdk/blob/cf85edeca422b52279df7dcba1675b5dbf5c4e75/src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp#L416 

`clone_instance_in_runtime` should only be called when cloning an instance. So in this context the is_array must be false. 

But it does seem strange that `ary_ptr != nullptr` is also in the condition. We could incorrectly call `clone_instance_in_runtime` if `ary_ptr == nullptr`. But I am not sure what that means.  It must be understood for this abstraction to make sense. If `ary_ptr != nullptr` is invariant then the code should be changed to:


-  if (ac->is_clone_array() && ary_ptr != nullptr) {
+  if (ac->is_clone_array()) {
+.   assert(ary_ptr != nullptr, "invariant");


and if it is not invariant. Then simply calling `clone_instance_in_runtime` is wrong. It must either be changed (and renamed) to handle `CloneArray.` or `clone_at_expansion` must do something else for that case.

Similarly I assume that the `ArrayCopyNode` is either `CloneInstance` or `CloneArray.` But there is also the `CloneOopArray` which I assume is not used given that we have the following code for `CloneArray`.
https://github.com/openjdk/jdk/blob/cf85edeca422b52279df7dcba1675b5dbf5c4e75/src/hotspot/share/gc/z/c2/zBarrierSetC2.cpp#L416-L420

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

PR Comment: https://git.openjdk.org/jdk/pull/19311#issuecomment-2124010965