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

[Aleksey Shipilev]

On Thu, 6 Apr 2023 16:50:00 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

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

Yup, that is deliberate to keep upstream diff cleaner, see https://github.com/openjdk/shenandoah/pull/245#discussion_r1160047465

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

Right, I removed that in new commit.

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

Cool, I refined the TODO with `or remove is 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).

Good! This should be done separately, removing this TODO then, right? I would be afraid about doing non-conservative changes in cleanup PR like this.

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

What is non-cosmetic in removing `oop newVal`? It does not seem to be used anywhere. Maybe it would indeed be needed for IU, let's reintroduce it then?

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160082916
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160082520
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160084062
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160082196
PR Review Comment: https://git.openjdk.org/shenandoah/pull/245#discussion_r1160081411