RFR (XS): 8183397: Ensure consistent closure filtering during evacuation

[Erik Helin]

On 07/06/2017 10:28 AM, Thomas Schatzl wrote:
> Hi Erik,
>
> On Thu, 2017-07-06 at 10:20 +0200, Erik Helin wrote:
>> On 07/03/2017 01:24 PM, Thomas Schatzl wrote:
>>>
>>> Hi all,
>> Hi Thomas,
>>
>>>
>>>   can I have reviews for this change that fixes an observation that
>>> has
>>> been made recently by Erik, i.e. that the "else" part of several
>>> evacuation closures inconsistently filters out non-cross-region
>>> references before checking whether the referenced object is a
>>> humongous
>>> or ext region.
>>>
>>> This causes somewhat hard to diagnose performance issues, and
>>> earlier
>>> filtering does not hurt if done anyway.
>>>
>>> (Note that the current way of checking in all but the UpdateRS
>>> closure
>>> using HeapRegion::is_in_same_region() seems optimal. The only
>>> reason
>>> why the other way in the UpdateRS closure is better because the
>>> code
>>> needs the "to" HeapRegion pointer anyway)
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8183397
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8183397/webrev/
>> -  } else if (in_cset_state.is_humongous()) {
>> +  } else {
>> +    if (in_cset_state.is_humongous()) {
>>
>> Why change `else if` to `else { if (...) {` here? Does it result in
>> the
>> compiler generating faster code for this case?
>
>   no. It only makes this do_oop_*() method look similar in structure to
> our do_oop_*() methods in the closures.
>
> I.e.
>
> if (in_cset.state.is_in_cset()) {
>   // do stuff for refs into cset
> } else {
>   // expanding handle_non_cset_obj_common()
>   if (state.is_humongous()) {
>   } else ...
> }
>
> I felt this improves overall readability, but this may only be because
> I have been working in this code a lot recently. I can revert this
> change.

Yeah, I suspected this was your reasoning. IMO, the code is a bit too 
spread out for this to work here, a reader of 
g1ParScanThreadState.inline.hpp might not be aware of the idioms used is 
g1OopClosures.inline.hpp.

So, for me, please use `else if` in g1ParScanThreadState.inline.hpp :) I 
do not need to re-review that change.

Great work Thomas, thanks!
Erik

> Thanks for your review,
>   Thomas
>