RFR: Enforce max regions [v3]

[Kelvin Nilsen]

On Wed, 7 Dec 2022 20:17:07 GMT, Y. Srinivas Ramakrishna <ysr at openjdk.org> wrote:

>> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix white space and add an assertion
>
> src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 198:
> 
>> 196: 
>> 197:   if (heap->mode()->is_generational()) {
>> 198:     // Since we probably have not yet reclaimed the most recently selected collection set, we have to defer
> 
> I'd make the comment less tentative, and state:
> 
> // Since the most recently selected collection set may not have been reclaimed at this stage,
> // we'll defer unadjust_avaliable() until after the full gc is completed.
> 
> Question: is the adjusted available value (modulo the loaned size) used by full gc for any purpose, or is it to satisfy assertion checks / verification in some of the methods invoked during full gc work below?

It's not used by full GC, but the Shenandoah verifier which runs at the start of full gc and then again at the end of full gc enforces compliance with the adjusted budgets.

The verifier didn't used to care.  But I made it care with this commit, and then I had to change where we do the unadjusting...

> src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp line 924:
> 
>> 922: size_t ShenandoahGeneration::decrement_affiliated_region_count() {
>> 923:   _affiliated_region_count--;
>> 924:   return _affiliated_region_count;
> 
> Both these seem fine and probably more readable, but you'd save a line by returning the pre-{in,de}cremented result, e.g.:
> 
> `return  --_affiliated_region_count;` 
> 
> Would it be useful to assert that the region count is always non-zero?

Actually, affiliated region count can be zero.  Often starts out that way for old-gen.

> src/hotspot/share/gc/shenandoah/shenandoahGeneration.hpp line 169:
> 
>> 167:   void scan_remembered_set(bool is_concurrent);
>> 168: 
>> 169:   size_t increment_affiliated_region_count();
> 
> Add a single line comment in the header file describing what a method returns:
> 
> // Returns the affiliated region count following the operation.

Thanks.

> src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 343:
> 
>> 341: };
>> 342: 
>> 343: class ShenandoahCalculateRegionStatsClosure : public ShenandoahHeapRegionClosure {
> 
> A one-line documentation spec here would be useful:
> 
> // A closure used to accumulate the net used, committed, and garbage bytes, and number of regions;
> // typically associated with a generation in generational mode.

Thanks.

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

PR: https://git.openjdk.org/shenandoah/pull/179