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

[Thomas Schatzl]

Hi,

On Tue, 2014-12-09 at 23:04 -0500, Kim Barrett wrote:
> 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?

Added.

> 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.
> 
> ------------------------------------------------------------------------------

The purpose of this method was to set the tenuring threshold to 0 in the
policy to avoid the code trying to continue failing to allocate in the
survivor space. This global state has been superseeded by thread-local
tenuring thresholds.

Now we have per-thread tenuring thresholds which are better in a few
ways:

- allocation failure of a particular thread in survivor does not mean
that all space is exhausted. It may allow other threads to continue
allocating for a bit, decreasing the number of objects that are promoted
eagerly.
- access is more local, i.e. everything referenced by 'this' should be
available anyway.

> 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.
> 

I agree, fixed for the methods that have been changed. This has been a
copy&paste of old code without further thought.

> ------------------------------------------------------------------------------
> 
> 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?

Change of visibility. The clear method in the super class is protected,
but the clear() method in this class is public.

It would be possible to use "G1BiasedMappedArray::clear()" without the
template parameter.

I can create a separate CR for this, removing this for all occurrences.

> ------------------------------------------------------------------------------
> 
> 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.

This has been removed in the current change. 

I would prefer to create a separate CR for introducing an array_size()
method.
Instead of this array_size() kludge I would actually prefer to have an
array wrapper class that automatically adds asserts for bounds checks.

As mentioned in the response to Mikael in this thread there are two
options for this change. I reuploaded them at:

Full, review changes only:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.1a
Diff from version 0:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.0_to_1a

Full changes merging in_cset_state_t and InCSetState into a single
struct:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.1b
Diff from version 1a:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.1a_to_1b

Thanks,
  Thomas