RFR: Cleanups, TODOs, asserts (part 1) [v2]

[Kelvin Nilsen]

On Wed, 5 Apr 2023 16:36:01 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:
>>  - [x] macos-aarch64-server-fastdebug (some pre-existing failures)
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   New assert fires, but instead of fixing it, remove useless live_memory completely

Marked as reviewed by kdnilsen (Committer).

src/hotspot/share/gc/shenandoah/c2/shenandoahBarrierSetC2.cpp line 900:

> 898:     } else {
> 899:       return true;
> 900:         }

As formatted in the github review display, this brace does not align properly.

src/hotspot/share/gc/shenandoah/c2/shenandoahBarrierSetC2.cpp line 943:

> 941:     int flags = ShenandoahHeap::HAS_FORWARDED;
> 942:     if (ShenandoahIUBarrier) {
> 943:       // TODO: Shouldn't this be OLD_MARKING too?

As currently implemented, generational mode does not support IU barrier.  Generational required SATB barrier.

YOUNG_MARKING is aka "traditional single-generation marking".

At some future time, we may add IU support to generational mode.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 58:

> 56: const double ShenandoahAdaptiveHeuristics::MAXIMUM_CONFIDENCE = 3.291; // 99.9%
> 57: 
> 58: // TODO: Provide comment here

I think recent "queued up" changes may not make use of this variable.  IIRC, this TODO might become: remove this variable if no longer needed.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahHeuristics.cpp line 81:

> 79:   ShenandoahHeap* heap = ShenandoahHeap::heap();
> 80:   if (!heap->mode()->is_generational()) {
> 81:     // TODO: Do we need this check, or assert is enough?

We should replace this with an assert() and fix up any invokers if they call inappropriately (to cause the assert failure).

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

> 1: /*

Almost all changes in this PR are "cosmetic".   Some of the changes in ShenandoahBarrierSet.inline.hpp appear to be beyond cosmetic.  Maybe this should be addressed in a separate PR.

Thinking out loud here: is the new_value argument for write_ref_field_post() only needed for IU-style post-write barriers?

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

PR Review: https://git.openjdk.org/shenandoah/pull/245#pullrequestreview-1375243011
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160040048
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160041997
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160044282
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160047999
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160070857