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

[sangheon.kim at oracle.com]

On 3/10/20 4:35 PM, Kim Barrett wrote:
>> 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.
Right, I thought that method is modified but it isn't.

>
>>   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.
I don't want argue with this, because removing/adding vs. changing its 
name/semantics seems same to me.
:)

>
> 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.
It is straightforward to me that the old method was just setter so 
reflected the member name. However if you intended to name/implement it 
with the concept of a functional interface, that is totally fine with me.
Hopefully I'm the only person who is questionable on the method name. :)

Thanks,
Sangheon

>