RFR: 8239825: G1: Simplify threshold test for mutator refinement

[Kim Barrett]

> On Mar 10, 2020, at 5:17 PM, sangheon.kim at oracle.com wrote:
> 
> Hi Kim,
> 
> On 3/3/20 6:16 PM, Kim Barrett wrote:
>> Please review this change to the handling of "padding" for the threshold
>> used to decide whether a mutator thread should perform concurrent
>> refinement.  Rather than doing a slightly tricky (because of potential
>> overflow) computation every time a mutator thread completes a buffer,
>> instead perform that computation once and record the result for repeated
>> use.
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8239825
>> 
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8239825/open.00/
> Looks good as is.

Thanks, but see below.

> src/hotspot/share/gc/g1/g1DirtyCardQueue.hpp
>  326   // Artificially increase mutator refinement threshold.
>  327   void set_max_cards_padding(size_t padding);
> - This method is not used even 8240133 and 8139652. I'm okay leaving as is if you think it may be used in the future.

It's called twice by G1ConcurrentRefine::adjust, at
g1ConcurrentRefine.cpp:404/406. Those lines didn't need to be changed,
because the functional behavior didn't change, just the underlying
implementation; see below.

>  330   void discard_max_cards_padding();
> - You changed the member name from '_max_cards_padding' to '_padded_max_cards' but it is not reflected on the method name.
>    Is this intended? I don't need a new webrev for this change if you would like to change it.

I didn't change the name of a data member, I removed one and added the
other; those two members have completely different semantics.

The old _max_cards_padding was the current amount of padding.  The
effective threshold was _max_cards + _max_cards_padding, being careful
to deal with overflow.

The new _padded_max_cards is the effective threshold, recomputed when
either _max_cards or the padding value is updated (being careful to
deal with overflow at that update time, rather than every time the
threshold is needed).  It has no accessor functions; it is a private
implementation detail, only used internally, where it is used directly
as a data member.

set_max_cards_padding(new_padding) still changes the current padding
value.  With this change that padding value is no longer directly
recorded in a data member.  Instead the padded threshold is computed
and recorded in the new data member (_padded_max_cards).  The ability
to make these kinds of implementation changes without changing the
external API is kind of the point of using a functional interface
rather than exposing data members to clients.

I think the name "set_max_cards_padding" doesn't (and shouldn't) imply
anything about the existence (or not) of a _max_cards_padding member.
I also don't think the public function name should be changed to
"set_padded_max_cards" to reflect the new member name, whose very
existence is an implementation detail.

The name could perhaps be changed to update_max_cards_padding, but I
don't think that's really an improvement.  What do others think.