RFR: 8270100: Fix some inaccurate GC logging

[Volker Simonis]

On Mon, 12 Jul 2021 18:50:32 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> `threads_count` and `adjust_for_thread_increase` get computed in `adjust_for_thread_increase()`. That's why they are passed in by reference. I don't think it makes sense to duplicate the computation of `threads_count` and `adjust_for_thread_increase` before we log them because the next time somebody will change `adjust_for_thread_increase()`, the values will be wrong again. This error occurred in the first place because [JDK-8144527](https://bugs.openjdk.java.net/browse/JDK-8144527) has factored out the computation of `threads_count` and `adjust_for_thread_increase` into the new  `adjust_for_thread_increase()` function, but did not pass the values back for logging.
>> 
>> For the second fix, I don't see why assigning to 100 to `_shrink_factor` and `current_shrink_factor` should be any better? These values aren't used except for logging if `ShrinkHeapInSteps` is false. Also, the assignment of `current_shrink_factor` can't easily be moved into the if-branch because `_shrink_factor` has to be reset to 0 before we potentially expand the heap (see comment `But if we recompute size without shrinking, it goes back to 0%.`). But on heap expansion, we'll return from the method early, before even reaching the shrinking logic.
>
>> duplicate the computation of threads_count and adjust_for_thread_increase
> 
> I don't view a method call and accessing a global as much duplication. Anyway, this is subjective.
> 
>> I don't see why assigning to 100 to _shrink_factor and current_shrink_factor should be any better?
> 
> Having a logic-free (does nothing but print) logger brings less surprise. IOW, many expect a logger faithfully reflect the actual internal states with no distortion.
> 
>> These values aren't used except for logging if ShrinkHeapInSteps is false.
> 
> That's true, but I prefer that the logger doesn't know `ShrinkHeapInSteps`, and just prints `_shrink_factor` and `current_shrink_factor` as they are.
> 
>> But on heap expansion, we'll return from the method early, before even reaching the shrinking logic.
> 
> I see; thank you for pointing it out. How about sth like this?
> 
> 
> void CardGeneration::compute_new_size() {
>   ... 
>   if (capacity_after_gc < minimum_desired_capacity) {
>     ...
>     // expanding the heap; reset shrink factor
>     _shrink_factor = 0;
>     return;
>   }
> 
>   if (capacity_after_gc > maximum_desired_capacity) {
>     ...
>     if (ShrinkHeapInSteps) {
>       current_shrink_factor = _shrink_factor;
>       _shrink_factor = ...
>     } else {
>       // Shrink 100% to the desired value
>       current_shrink_factor = _shrink_factor = 100;
>     }
>     // log internal states
>   }
> }

@albertnetymk  your latest proposal is still changing the current semantics. Before, the shrink factor was reset on every invocation of `CardGeneration::compute_new_size()`. With your proposal, it will only be reset if we expand the heap.

My patch is really just a trivial fix of some logging errors. I'm not against changing or improving it, but before I introduce any behavioral changes, I'd like to hear a second opinion.

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

PR: https://git.openjdk.java.net/jdk/pull/4727