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

[Kim Barrett]

On Dec 15, 2014, at 7:03 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> 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

------------------------------------------------------------------------------  
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
2766   return false; // Keep some compilers happy

The latest change set capitalized the first word of this comment, but
not all the very similar comments nearby.  It would be better to
change all or none, not just this one.

(I also think these returns should directly follow
ShouldNotReachHere() in the default clauses, rather than being after
the switch statement.  But that should be part of a separate cleanup,
especially since that later cleanup might be to remove such return
statements.)

------------------------------------------------------------------------------   
src/share/vm/gc_implementation/g1/g1InCSetState.hpp  

The name "is_not_in_cset" is unfortunate; I keep wanting to misread
that as meaning !is_in_cset.  I don't have a better suggestion that
doesn't involve a complete renaming of the type, and I don't have a
better type name to suggest.

Similarly, G1CollectedHeap::in_cset_state() is also confusing to me; I
keep wanting to misread that as a predicate.

------------------------------------------------------------------------------   
src/share/vm/gc_implementation/g1/g1InCSetState.hpp  
  71 class G1InCSetStateFastTestBiasedMappedArray : public G1BiasedMappedArray<in_cset_state_t> {
  72  protected:
  73   in_cset_state_t default_value() const { return InCSetState::NotInCSet; }

I think this should be private, not protected.  It's also only used in
asserts. 

==============================================================================

Below are for merging in_cset_state_t and InCSetState.  I don't have a
strong preference between the two options.

------------------------------------------------------------------------------   
src/share/vm/gc_implementation/g1/g1InCSetState.hpp 
  32 struct InCSetState {

Use of "struct" instead of "class" makes _value public.

------------------------------------------------------------------------------  
src/share/vm/gc_implementation/g1/g1InCSetState.hpp 
  78   bool is_valid() const                { return _value < Num; }

Shouldn't this also check lower bound, e.g. Humongous <= _value?

------------------------------------------------------------------------------    
src/share/vm/gc_implementation/g1/g1InCSetState.hpp  
  73   in_cset_state_t default_value() const { return InCSetState::NotInCSet; }

Unused function in this variation.

------------------------------------------------------------------------------     
src/share/vm/gc_implementation/g1/g1ParScanThreadState.hpp
  52   InCSetState   _dest[InCSetState::Num];

Whitespace separation between "InCSetState" and "_dest" doesn't
conform to surrounding code style.  Presumably a query-replace but
without whitespace adjustment.

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