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

[Jon Masamitsu]

Thomas,

Since I didn't have any unanswered question from my earlier
review, I was not planning on doing a further review of these
later changes.  Let me know if you need another review.

Jon

On 12/16/2014 03:57 AM, Thomas Schatzl wrote:
> 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
>