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

[Thomas Schatzl]

Hi Kim,

  thanks for keeping to look at this. Very appreciated.

On Mon, 2014-12-15 at 14:12 -0500, Kim Barrett wrote:
> 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.

Undid that. Let's do that another time.

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

In some different implementation I had region_attr_t as type name...

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

Required to implement G1BiasedMappedArray.

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

I will use the "b" variant then as I think it is cleaner (even if only by a little).

> 
> ------------------------------------------------------------------------------   
> src/share/vm/gc_implementation/g1/g1InCSetState.hpp 
>   32 struct InCSetState {
> 
> Use of "struct" instead of "class" makes _value public.

Added explicit visibility specifier.

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

Fixed.

> ------------------------------------------------------------------------------    
> src/share/vm/gc_implementation/g1/g1InCSetState.hpp  
>   73   in_cset_state_t default_value() const { return InCSetState::NotInCSet; }
> 
> Unused function in this variation.
> 

Same as above.

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

Fixed.

Diff webrev:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.1b_to_2b/
Full webrev:
http://cr.openjdk.java.net/~tschatzl/8060025/webrev.2b/

Thanks!

Thomas