RFR: Fix satb barrier for object array copy

[Aleksey Shipilev]

On Wed, 29 Mar 2023 23:54:25 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

> The satb barrier for overwritten arrays residing in old-gen must be unconditional during young GC.
> 
> During old GC, the satb barrier is only needed for overwritten old-gen arrays if the array is below TAMS within its old-gen region.

I have suggestions :)

Edit: Apologies, have not realized this is a draft PR, so I reviewed it as if it was final. Still, consider these suggestions!

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 431:

> 429: 
> 430: template <class T>
> 431: void ShenandoahBarrierSet::arraycopy_marking(T* src, T* dst, size_t count, bool is_old) {

Oh, this `is_old` is not about the object, it is about the marking! Maybe name it `is_old_marking`?

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 458:

> 456:     if (is_old) {
> 457:       ShenandoahHeapRegion* r = _heap->heap_region_containing(array);
> 458:       assert(_heap->mode()->is_generational(), "Only perform old-marking if in generational mode");

Let's say:

Suggestion:

    if (is_old) {
      // Generational, old marking
      assert(_heap->mode()->is_generational(), "Invariant");
      ShenandoahHeapRegion* r = _heap->heap_region_containing(array);

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 459:

> 457:       ShenandoahHeapRegion* r = _heap->heap_region_containing(array);
> 458:       assert(_heap->mode()->is_generational(), "Only perform old-marking if in generational mode");
> 459:       if (r->is_old() && (((HeapWord*) array)) < _heap->marking_context()->top_at_mark_start(r)) {

There is a missing `reinterpret_cast<HeapWord*>(array)` here. Probably pull it into the local, so that every branch can just reuse it.

Also, this check is `!_heap->marking_context()->allocated_after_mark_start(reinterpret_cast<HeapWord*>(array))`?

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 463:

> 461:       }
> 462:     } else if (_heap->mode()->is_generational()) {
> 463:       ShenandoahHeapRegion* r = _heap->heap_region_containing(array);

Suggestion:

    } else if (_heap->mode()->is_generational()) {
      // Generational, young marking
      ShenandoahHeapRegion* r = _heap->heap_region_containing(array);

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.inline.hpp line 467:

> 465:         arraycopy_work<T, false, false, true>(array, count);
> 466:       }
> 467:     } else {

Suggestion:

    } else {
      // Non-generational
      assert(!_heap->mode()->is_generational(), "Invariant");

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 325:

> 323: 
> 324:     phase4_compact_objects(worker_slices);
> 325: 

This is the part of #235, is it? Should not be in this PR, I think.

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

PR Review: https://git.openjdk.org/shenandoah/pull/237#pullrequestreview-1364570502
PR Review Comment: https://git.openjdk.org/shenandoah/pull/237#discussion_r1152907365
PR Review Comment: https://git.openjdk.org/shenandoah/pull/237#discussion_r1152920559
PR Review Comment: https://git.openjdk.org/shenandoah/pull/237#discussion_r1152904283
PR Review Comment: https://git.openjdk.org/shenandoah/pull/237#discussion_r1152920891
PR Review Comment: https://git.openjdk.org/shenandoah/pull/237#discussion_r1152921627
PR Review Comment: https://git.openjdk.org/shenandoah/pull/237#discussion_r1152897354