RFR (L): 8060025: Object copy time regressions after JDK-8031323 and JDK-8057536

[Kim Barrett]

On Dec 3, 2014, at 7:41 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8060022
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8060025/webrev.0/
> Testing:
> JPRT, specjbb2005/2013, CRM Fuse


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

src/share/vm/gc_implementation/g1/g1Allocator.hpp
  45   static in_cset_state_t humongous() { return -1; }

Is there a reason for this function, rather than adding a Humongous
tag to the InCSetState enum?

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

src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
Removed:
6547     } else {
6548       g1_policy()->note_alloc_region_limit_reached(ap);

I haven't been able to figure out why this (adjusted for purpose =>
InCSetState) was removed.

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

src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp
 558       default:
 559         assert(false, err_msg("Unknown dest: %d", dest));
 560         break;
 561     }
 562     // keep some compilers happy
 563     return NULL;

I think better would be:

    default:
      ShouldNotReachHere();
      return NULL;    // keep some compilers happy

I think ShouldNotReachHere() is preferable to an assert(false,...)
because the former may expand to a "unreachable" or "noreturn"
indicator understood by the compiler.  [For example, in a release
build, it could be argued that the gcc version of ShouldNotReachHere()
should expand to __builtin_unreachable(), likely resulting in better
code here.]

I'd prefer to keep the compiler silencing closer to the source of the
issue, so that some other nearby future change doesn't accidentally
make it reachable.

There are a number of similar occurrances of just the first or of both
of these elsewhere in this change set.

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

src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp
1346     void clear() { G1BiasedMappedArray<in_cset_state_t>::clear(); }

Why add this trivial forwarding definition, and not just use the
inherited definition as is?

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

src/share/vm/gc_implementation/g1/g1ParScanThreadState.hpp
  88     assert(index < InCSetState::Num,

Instead of InCSetState::Num, consider

  sizeof(_dest) / sizeof(_dest[0])

And maybe we should add the usual array_size() utility to
globalDefinitions.hpp.  A quick search of hotspot sources shows many
places where it would be appropriate to use.

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