RFR (S): 8073466: Remove buffer retaining functionality and clean up in ParGCAllocBuffer

[Jon Masamitsu]

Thomas,

Looks good.

Reviewed.

Some comments below but nothing important.

Jon


On 03/02/2015 04:11 AM, Thomas Schatzl wrote:
> Hi Jon,
>
>    thanks for the review :)
>
> On Fri, 2015-02-27 at 16:26 -0800, Jon Masamitsu wrote:
>> Thomas,
>>
>> http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1/src/share/vm/gc_implementation/shared/parGCAllocBuffer.cpp.frames.html
>>
>> Could you add a comment here that the stats are being "flushed".
>> Something like
>>
>> // Flush the statistics.
>>>    67   stats->add_allocated(_allocated);
>>>     68   // Retire() counts the unused space as wasted. So we need to remove it again
>>>     69   // before updating the statistics.
>>>     70   stats->add_wasted(_wasted - unused);
> Done.

Thanks.

>
>> In a context where the unused space at the end of a buffer might be used
>> later (as when there was the "retaining functionality"),  I can see that
>> you might want to differentiate between _wasted and unused.    Since there
>> is no longer a "retain", it seems all to be just waste.   So
>> I don't see the use of passing "_wasted - unused" as opposed to just
>> "_wasted".
> There is still use of "_unused" in the calculation of the desired PLAB
> size. I tried to avoid changing the current PLAB sizing in this change,
> and later for G1 I plan to override the desired_plab_size() method.
>
> [This is needed in any case because of improved statistics for G1, where
> we want to distinguish between waste within the PLABs and waste caused
> by refill due to the current PLAB not fitting into the current region.]

Ok.  That's reasonable.

>
> The existing PLAB sizing calculates the amount of "target_refills" using
> the _unused member. I admit I do not really understand that calculation,
> and the results seem bogus in particular in presence of multiple threads
> each having its own PLAB and considering the integer truncation in the
> formula.
>
> I can create a CR to remove _unused and fix the CMS PLAB sizing.

I don't know what "fix the CMS PLAB sizing" means but
if you put it into a CR, I'll know.

>
>> If you would like to keep the differentiation between "_wasted - used"
>> and "_wasted", consider adding back the "end_of_gc" flag to
>> retire() and moving
>>
>>>    127     _wasted += pointer_delta(_end, _top);  // unused  space
>> out of invalidate() and into retire() in this fashion
>>
>> if (!end_of_gc) {
>>     _wasted += pointer_delta(_end, _top);  // unused  space
>> }
> Done. I did not use an additional parameter to retire, as we already
> have two retiring methods (retire and flush_stats_and_retire) to be
> called at different times during GC anyway.

Thanks.

>
> Also, please ignore that G1 at the moment calls flush_stats_and_retire()
> every time it allocates a G1ParScanThreadState. This is basically
> JDK-8040162. I will ask for a review of that soon.

OK.

>
>> Throwaway suggestion (your call, since not in your change)
>>
>> Fix the indentation on the fields
>>
>>>     39   char head[32];
>>>     40   size_t _word_sz;          // In HeapWord units
>>>     48   char tail[32];
> Fixed.
>
> http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1_to_2/

Looks good.

Reviewed.

Jon

> (incremental)
> http://cr.openjdk.java.net/~tschatzl/8073466/webrev.2/ (full)
>
> (The change also moves invalidate() to the protected section of
> ParGCAllocBuffer).
>
> Thanks,
>    Thomas
>
>