RFR: Cleanups, TODOs, asserts (part 2)

[Y. Srinivas Ramakrishna]

On Tue, 11 Apr 2023 15:00:19 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

> I have been reading the openjdk/shenandoah vs openjdk/jdk diff and these are the simple code massages we can do to improve and bullet-proof the code, hopefully without changing the semantics. This would continue with more parts later.
> 
> Additional testing:
>  - [ ] macos-aarch64-server-fastdebug (some pre-existing failures)
>  - [ ] GHA builds

A few minor comments, but otherwise looks good.

src/hotspot/share/gc/shenandoah/shenandoahMarkingContext.hpp line 63:

> 61: 
> 62:   // Simple versions of marking accessors, to be used outside of marking (e.g. no possible concurrent updates)
> 63:   // TODO: Do these really need to be const?

It's generally considered good hygiene to `const` read accessors. Any reason you would _not_ want them `const`'d?

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

> 38: 
> 39: public:
> 40:   // TODO: Have a separate memory pool implementation for non-generational

Not sure about this.

src/hotspot/share/gc/shenandoah/shenandoahOldGeneration.hpp line 106:

> 104: private:
> 105:   bool entry_coalesce_and_fill();
> 106:   bool coalesce_and_fill();

Any reason to leave these private methods here, rather than move them to the `private` section immediately following the (related) private data-members at (new) lines 35-37 above?

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 149:

> 147:               "oop must be aligned");
> 148: 
> 149:     ShenandoahHeapRegion *obj_reg = _heap->heap_region_containing(obj);

Stet. Looks like this change should not be made, nor the ones below marked "Ditto".

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 160:

> 158:              "Object klass pointer must go to metaspace");
> 159: 
> 160:       HeapWord *obj_addr = cast_from_oop<HeapWord*>(obj);

Ditto

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 225:

> 223:              "Should have no humongous forwardees");
> 224: 
> 225:       HeapWord *fwd_addr = cast_from_oop<HeapWord *>(fwd);

Ditto

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 585:

> 583:   const char* _label;
> 584:   ShenandoahVerifier::VerifyOptions _options;
> 585:   ShenandoahHeap *_heap;

STET. This change shouldn't be made, nor a few more of this kind below, marked with "Ditto".

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 645:

> 643:   }
> 644: 
> 645:   virtual void work_humongous(ShenandoahHeapRegion *r, ShenandoahVerifierStack& stack, ShenandoahVerifyOopClosure& cl) {

Ditto

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 654:

> 652:   }
> 653: 
> 654:   virtual void work_regular(ShenandoahHeapRegion *r, ShenandoahVerifierStack &stack, ShenandoahVerifyOopClosure &cl) {

Ditto

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 687:

> 685:   }
> 686: 
> 687:   void verify_and_follow(HeapWord *addr, ShenandoahVerifierStack &stack, ShenandoahVerifyOopClosure &cl, size_t *processed) {

Ditto

src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 727:

> 725:     // TODO: Also, only accept OLD_MARKING in generational mode.
> 726:     return (actual != expected) && !(actual & ShenandoahHeap::OLD_MARKING);
> 727:   }

This will read more naturally, if you reverse the boolean sense of the newly intriduced `verify...` method, so that:

if (!verify...()) {
  fatal(...);
}

with `verify...()`:

    return (actual == expected ) || (actual & OLD_MARKING);

src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp line 66:

> 64: 
> 65:     // Old objects should be registered and RS cards within *read-only* RS are dirty for all
> 66:     // interesting pointers.

I'd generally prefer replacing the vague "interesting" (everywhere) with the more precise "inter-generational" :-)

src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp line 67:

> 65:     // Old objects should be registered and RS cards within *read-only* RS are dirty for all
> 66:     // interesting pointers.
> 67:     _verify_remembered_for_marking,

Since we are talking naming here, I'd personally prefer (but will defer to existing protocol) replacing `for` with `before`, much like we use `after` in some cases. This would be in line with Dijkstra- or Hoare-like `pre-` and `post-` terminology.

src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp line 70:

> 68: 
> 69:     // Old objects should be registered and RS cards within *read-write* RS are dirty for all
> 70:     // interesting pointers.

ditto

src/hotspot/share/gc/shenandoah/shenandoahVerifier.hpp line 74:

> 72: 
> 73:     // Old objects should be registered and RS cards within *read-write* RS are dirty for all
> 74:     // interesting pointers.

ditto

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

Changes requested by ysr (Author).

PR Review: https://git.openjdk.org/shenandoah/pull/252#pullrequestreview-1379768020
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163126438
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163141337
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163534055
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163541666
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163542153
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163542291
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163546176
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163546259
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163546316
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163546356
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163550163
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163564302
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163566796
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163564677
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163564895