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

Jon Masamitsu jon.masamitsu at oracle.com
Sat Feb 28 00:26:21 UTC 2015


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);

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

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
}

http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1/src/share/vm/gc_implementation/shared/parGCAllocBuffer.hpp.frames.html

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];

Jon

On 2/27/2015 3:58 AM, Thomas Schatzl wrote:
> Hi all,
>
>    I would like to ask for one Reviewer review.
>
> Current webrevs are at:
>
> http://cr.openjdk.java.net/~tschatzl/8073466/webrev.1/
> http://cr.openjdk.java.net/~tschatzl/8073466/webrev.0_to_1/
>
> Thanks,
>    Thomas
>
> On Fri, 2015-02-20 at 15:40 +0100, Thomas Schatzl wrote:
>> Hi all,
>>
>>    can I have reviews for the following cleanup? It removes the buffer
>> retain functionality of ParGCAllocBuffer.
>>
>> It has (once I guess, but actually I am not sure because that must have
>> been long time ago) been used to retain the space at the end of the PLAB
>> for the next GC.
>>
>> Nobody uses it. So that and all related code can be removed.
>>
>> Here is a long description of changes:
>>
>> ParGCAllocBuffer::flush_stats_and_retire() is always called with true
>> for "end_of_gc" and false for the "retain" parameter.
>> When moving that into that call to retire(), you will notice that
>> retire() is always called with retain=false.
>>
>> Looking at ParGCAllocBuffer::retire(), if retain is always false,
>> _retained will never be true, which means that _retained_filler is never
>> used. With _retained always false, all of the asserts in retire() always
>> evaluate to true, so the end_of_gc parameter is useless.
>>
>> (Actually the whole _retained_filler is never loaded from, making this
>> entire functionality non-working in the first place).
>>
>> Now looking at what retire() does, there is actually a bug in
>> flush_stats_and_retire() because retire() adds to the waste (the area
>> between _top and _hard_top), but flush() is called first.
>>
>> Looking at the code, we also only flush stats when ResizePLAB is set,
>> due to performance concerns. However this means that the output provided
>> by PrintPLAB will not be updated if ResizePLAB is not set, which is not
>> desirable at all (for comparison of fragmentation with ResizePLAB
>> enabled). I did not see the three atomic adds as a performance problem,
>> as this method is called once per PLAB per GC.
>>
>> There is a small additional change in PLABStats::adjust_desired_plab_sz
>> where I merged the two separate "if (PrintPLAB)" statements.
>>
>> Other cleanup changes include:
>>    - removed an obscure "XXX" comment
>>    - removed a PRAGMA
>>    - cleaned up includes (BlockOffsetTable - huh?)
>>    - tried to improve comments
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8073466
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8073466/webrev/
>> testing:
>> jprt, perf benchmarks with G1 and CMS
>>
>> Thanks,
>>    Thomas
>>
>>
>



More information about the hotspot-gc-dev mailing list