RFR: Cleanups, TODOs, asserts (part 2)

[Aleksey Shipilev]

On Wed, 12 Apr 2023 03:35:16 GMT, Y. Srinivas Ramakrishna <ysr 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:
>>  - [x] macos-aarch64-server-fastdebug (some pre-existing failures)
>>  - [x] GHA builds
>
> 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);

Right! Fixed.

> 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.

Right, did the renames to "before". Also moved the verification code from `ShenandoahHeap` to `ShenandoahVerifer`, since it logically belongs there.

But even the "before" thing deviates from the intent for `_verify_*` enums. They describe the _property_ we want to check, rather than the binding to a concrete phase. So it should probably be something like `_verify_rset_readonly` / `_verify_rset_readwrite_marked`? I'll leave that for future cleanup, along with cleanups in rset verification code.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163818825
PR Review Comment: https://git.openjdk.org/shenandoah/pull/252#discussion_r1163816464