RFR: 8321939: [GenShen] ShenandoahOldEvacRatioPercent=100 fails with divide-by-zero [v2]

[Kelvin Nilsen]

On Wed, 13 Dec 2023 01:39:47 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> The code that used the option ShenandoahOldEvacRatioPercent (SOEP) didn't correctly deal with the extremal setting of 100. Corrected the error and did some light refactoring in the vicinity, getting rid of some dead variables, adding documentation, and assertions. Also modified slightly the documentation of the option.
>> 
>> **Testing:**
>> - [x] GHA
>> - [ ] code pipeline testing
>> - [ ] specjbb with SOEP=default,0,100 to verify that the issue is fixed
>
> Y. Srinivas Ramakrishna has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix a scope issue with a local variable.

Marked as reviewed by kdnilsen (Committer).

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 269:

> 267:   //            SOEP/100 = OE/TE
> 268:   //                     = OE/(OE+YE)
> 269:   //  => SOEP/(100-SOEP) = OE/((OE+YE)-OE)

I get lost at this step.  Here's how I see the algebra from this point forward:
  (OE + YE) * SOEP == 100 OE         (multiply both sides by 100 (OE + YE)
  YE * SOEP = 100 OE - SOEP * OE (subtract SOEP * OE from both sides)
  YE * SOEP = OE * (100 - SOEP)
  OE = YE * SOEP / (100 - SOEP)
  
  (we agree on final result.  I just don't follow the justification for step at line 269.  maybe you can explain that line better?)

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

> 1204:   //                     = OE/YE
> 1205:   //  =>              OE = YE*SOEP/(100-SOEP)
> 1206: 

Same comment as above, regarding the algebra steps.

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

> 1240:   const bool doing_promotions = promo_load > 0;
> 1241:   if (doing_promotions) {
> 1242:     // We're only promoting and we have a maximum bound on the amount to be promoted

With reference to the preceding comment, we should probably add an assert here: assert(!doing_mixed, "Don't do mixed and promotions in the same cycle")

Or maybe this is no longer true.  It doesn't have to be true.  The code that follows seems to suggest this is no longer true.  It was a simplification at some point in the implementation history.  If it is no longer true, let's remove that comment.

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

PR Review: https://git.openjdk.org/shenandoah/pull/369#pullrequestreview-1780476196
PR Review Comment: https://git.openjdk.org/shenandoah/pull/369#discussion_r1425876097
PR Review Comment: https://git.openjdk.org/shenandoah/pull/369#discussion_r1425879329
PR Review Comment: https://git.openjdk.org/shenandoah/pull/369#discussion_r1425893069