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