RFR (S): JDK-8028710: G1 does not retire allocation buffers after reference processing work

[Bengt Rutisson]

Hi Thomas,

Nice change. Looks good.

Moving retire_alloc_buffers() to the destructor of G1ParScanThreadState 
is nice, but has the side effect that in 
G1CollectedHeap::process_discovered_references() the time to retire the 
alloc buffers is not included anymore in the reference processing time 
reported by that method. I guess this time is bounded and hopefully 
short, but do you think it could be an issue?

I assume you had to change the initial value of _retired in 
G1ParGCAllocBuffer from false to true because of the assert in 
~G1ParGCAllocBuffer(). Can you elaborate a bit on why we create a 
G1ParGCAllocBuffer without using and retiring it? Basically, why is the 
initial value for _retired not false?

Nice that there were no performance impact of making the methods in 
ParGCAllocBuffer virtual.

Thanks,
Bengt

On 2014-04-11 12:37, Thomas Schatzl wrote:
> Hi all,
>
>    can I have some reviews for the following bugfix?
>
> When doing parallel reference processing, its allocation buffers are not
> retired. This causes problems with the not-retired buffer being wrongly
> formatted (however I could not trigger a bug because of that since that
> area does not contain live objects, and G1 always uses one of the
> liveness bitmaps for walking the heap).
>
> Further, and typically more importantly, it inflates the PLAB sizes as
> the space wasted by these buffers (typically a lot compared to the
> actually number of copied bytes) is not accounted for when PLAB resizing
> occurs.
>
> You may notice that this CR adds the check that the PLABs must be
> retired (in G1ParGCAllocBuffer::~G1ParGCAllocBuffer): this is because
> when the problem had been identified, there has been one, which was part
> of JDK-6976350. That change has been reverted since though.
>
> The change further fixes a few areas which lead to that issue: first
> G1ParGCAllocBuffer hides but not overrides the set_buf() and retire()
> methods, which, when the latter is called via
> ParGCAllocBuffer::flush_stats_and_retire() leads to a call of the wrong
> retire() method. So the _retired member of G1ParGCAllocBuffer is never
> set to true.
> So these two methods were made virtual in ParGCAllocBuffer.
>
> Another issue fixed is that the initial value of _retired should be
> true, as buffers are assigned lazily.
>
> Finally, to avoid missing calls to
> ParGCAllocBuffer::retire_alloc_buffers(), I put the call to it into the
> destructor of G1ParScanThreadState so that it does not need to be called
> manually any more (which means that its visibility changed to private).
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8028710/webrev/
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8028710
>
> Testing:
> no perf regressions on any standard benchmark without parallel reference
> processing with g1 and pargc on a few platforms, jprt
>
> Thanks,
> Thomas
>