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

[Thomas Schatzl]

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.

Thanks for your review,
  Thomas