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

[Mikael Gerdin]

Thomas,

On 2014-12-15 13:03, Thomas Schatzl wrote:
> 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

I'd prefer 1b, SPARC hardware usually boasts a large amount of memory so 
they can probably afford the footprint regression.
It makes the code a lot nicer.

/Mikael

>
> Thanks,
>    Thomas
>
>