RFR: Scan remembered

[Roman Kennke]

On Fri, 8 Jan 2021 17:56:02 GMT, Kelvin Nilsen <github.com+51720475+kdnilsen at openjdk.org> wrote:

> Add support for scanning remembered set
> 
> The code has support for two alternative implementations of the remembered set. The current remembered set implementation uses traditional card marking, where the post writer barrier for pointer write operations sets the mark for every overwritten card.
> 
> A contemplated future remembered set implementation is represented in skeleton form within the shenandoahBufferWithSATBRememberedSet.hpp and shenandoahBufferWithSATBRememberedSet.inline.hpp files. The idea of this alternative remembered set implementation is that the existing SATB buffers will be augmented to additionally remember the address of each overwritten reference field. Subsequent processing of the SATB buffer contents by background GC threads will update the TBD remembered set representation.
> 
> There are known bugs and performance improvements in the remembered set scanning implementation that have been addressed in certain Amazon-internal commits. These commits will be upstreamed at a later time after other commits not directly related to remembered set scanning are upstreamed.

Nice work, thanks! I like that it explains so many details in comments. I've found a few issues, see below.

src/hotspot/share/gc/shenandoah/shenandoahBufferWithSATBRememberedSet.hpp line 1:

> 1: //

New file lacks copyright notice.

src/hotspot/share/gc/shenandoah/shenandoahBufferWithSATBRememberedSet.hpp line 17:

> 15: //      a) Just blindly dirty each card that is overwritten with a pointer value, regardless of the written value, as with
> 16: //         the implementation of traditional direct card marking.  When this card's memory region is eventually scanned, the
> 17: //         the implementation of remembered set scanning will clear the page if it no longer holds references to young-gen

Minor: double- 'the'

src/hotspot/share/gc/shenandoah/shenandoahBufferWithSATBRememberedSet.hpp line 61:

> 59: // existing SATB pre-write barrier to maintain remembered set information rather than using unconditional direct card marking in
> 60: // a post-write barrier.
> 61: class ShenandoahBufferWithSATBRememberedSet: public CHeapObj<mtGC> {

So, if I understand correctly, you intend to piggy-back the field-address onto the SATB queue, is that right? It would mean that we need to double the size of the items in the buffer. And it would also mean that SATB needs to be turned on all the time, not just during marking. Is that correct? If that is so, I wonder if it may be better to do like G1 does and use a separate queue and barrier for this? It looks to me that it is doing exactly what we need here.

src/hotspot/share/gc/shenandoah/shenandoahBufferWithSATBRememberedSet.inline.hpp line 1:

> 1: 

Missing copyright header too.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1159:

> 1157:     ShenandoahEvacuateUpdateRootsClosure<> cl;
> 1158:     MarkingCodeBlobClosure blobsCl(&cl, CodeBlobToOopClosure::FixRelocations);
> 1159: 

Extra whitespace

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2730:

> 2728:         //       phase do not need to be scanned by update-refs because the to-space invariant is in force
> 2729:         //       during evacuation and this will assure that any objects residing between
> 2730:         //       r->get_update_watermark() and r->top() hold no pointers to from-space.

We do need to scan objects that have been allocated by evacuation, because they may hold from-space refs. For this reason, we bump-up UWM when allocating for evac (in ShFreeSet, iirc).

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2720:

> 2718:         // an old-gen region as part of an old-gen collection.  Otherwise, a subseqent update-refs scan of
> 2719:         // the same region will see stale pointers and crash.
> 2720:         //

This is probably a bad idea. Unreachable objects are not guaranteed to have their Klass* intact, and may crash when accessed. Why would it crash during subsequent scans?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2741:

> 2739:         // allocate from within a free region.
> 2740:         //
> 2741:         // Reality is that this code is likely to be replaced with JVM-292 code before we ever get around to

What is 'JVM-292' code? ;-)

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 71:

> 69: class VMStructs;
> 70: 
> 71: 

Extra WS

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 40:

> 38: #include "gc/shenandoah/shenandoahScanRemembered.hpp"
> 39: #include "memory/metaspace.hpp"
> 40: #include "gc/shenandoah/shenandoahScanRemembered.hpp"

You're including the same header 2x

src/hotspot/share/gc/shenandoah/shenandoahHeap.hpp line 49:

> 47: class ShenandoahCollectorPolicy;
> 48: class ShenandoahControlThread;
> 49: class ShenandoahDirectCardMarkRememberedSet;

This looks unused.

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.hpp line 30:

> 28: 
> 29: #include "gc/shared/referenceDiscoverer.hpp"
> 30: #include "gc/shared/referencePolicy.hpp"

This looks not needed or, if it is actually needed, not related to this change.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 2:

> 1: /*
> 2:  * Copyright (c) Amazon.com, Inc. or its affiliates.  All rights reserved.

You do have the copyright header here, good! :-) But it's lacking the year, like so:
 * Copyright (c) 2021, Amazon.com, Inc. or its affiliates. All rights reserved.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 2:

> 1: /*
> 2:  * Copyright (c) Amazon.com, Inc. or its affiliates.  All rights reserved.

Missing copyright year, as above.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 103:

> 101: }
> 102: 
> 103: // Implementation is not correct.  ShenandoahBufferWithSATBRememberedSet is a placeholder for future planned improvements.

This should go into its own .cpp file I think. Actually, I am not sure that we should add empty&unused code at all. Maybe add it when you also fill the implementation and use?

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 90:

> 88: 
> 89: // The typical parallel remembered set scanning effort consists of the
> 90: // following steps, all of which are performed during a JVM safetpoint:

Typo: safetpoint -> safepoint.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 212:

> 210: // into ShenandoahRootEvacuator::roots_do() before the invocation of
> 211: // _serial_roots.oops_do(oops, worker_id).
> 212: 

This is not quite clear to me. Above you said that all of the work is done during safepoints. Here you mention 'concurrent scanning' ... what is true?

src/hotspot/share/gc/shenandoah/shenandoahBufferWithSATBRememberedSet.hpp line 61:

> 59: // existing SATB pre-write barrier to maintain remembered set information rather than using unconditional direct card marking in
> 60: // a post-write barrier.
> 61: class ShenandoahBufferWithSATBRememberedSet: public CHeapObj<mtGC> {

It's also lacking the usual #ifndef - #define - #endif sequence that is needed by headers.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 26:

> 24:  */
> 25: 
> 26: // Terminology used within this source file:

We usually place big comments like this below #includes and just above class declarations.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.hpp line 787:

> 785:   // What card number corresponds to old-gen heap addresss p.  (If p
> 786:   // does not refer to old-gen memory, the returned value is undefined.)
> 787:   uint32_t cardNoForAddr(HeapWord *p);

Should be named card_no_for_addr(HeapWord* p) to match style of most other Hotspot code.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.inline.hpp line 2:

> 1: /*
> 2:  * Copyright (c) Amazon.com, Inc. or its affiliates.  All rights reserved.

Missing copyright year.

src/hotspot/share/gc/shenandoah/shenandoahScanRemembered.cpp line 26:

> 24:  */
> 25: 
> 26: 

Make sure you're adding #include "precompiled.hpp" and make sure it builds with and without --disable-precompiled-headers

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

Changes requested by rkennke (Reviewer).

PR: https://git.openjdk.java.net/shenandoah/pull/12