RFR (S): 8152438: Threads may do significant work out of the non-shared overflow buffer

[Erik Helin]

On 2016-05-11, Thomas Schatzl wrote:
> Hi all,
> 
> On Wed, 2016-05-11 at 09:35 +0200, Thomas Schatzl wrote:
> > Hi all,
> > 
> >   can I have reviews for the following change that fixes signficiant
> > imbalance in work performed during GC?
> > 
> > We observed that threads in certain situations put a lot of
> > references
> > into the thread-private overflow queue. Since this overflow queue is
> > thread-local, and other threads may not steal work from this queue,
> > it
> > happens that sometimes the parallel copying phase is severely
> > serialized.
> > 
> > This is particularly problematic on large machines with many threads
> > but comparatively little work to do. In one situation on a real world
> > application, a 200% overall throughput improvement has been observed
> > by
> > this change.
> > 
> > The CR mentions a micro-benchmark that exhibits the same problems.
> > 
> > The proposed solution is to, for a particular thread, if it processes
> > entries from the overflow queue, first try to just move the work item
> > to the shared queue where other threads can steal from. Only process
> > the item directly if the shared queue is full already.
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8152438
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8152438/webrev/
> > Testing:
> > jprt, regular perf testing with no apparent pause time regressions,
> > some benchmarks
> 
>   had some offline discussion with Erik about the change, and the main
> gripe with the change has mainly been putting additional policy code
> about how the OverflowTaskQueue works somewhere completely unrelated,
> in this case G1ParScanThreadState::trim_queue().
> 
> Looking at other uses, we found that all of these do the same, and it
> would be much better to have collector/case specific
> "OverflowTaskQueue"s that encapsulate the behavior. This would also
> allow potential sharing or at least comparison of them.
> 
> We felt that looking into that is out of scope for this change (apart
> from one minor renaming of try_push()), and I filed JDK-8156754 for
> this other work.
> Another point came up was why not doing bulk-move of the queue elements
> from the overflow queue to the task queue. Due to already being late
> for FC, we also moved this to an RFE, JDK-8156739.
> 
> http://cr.openjdk.java.net/~tschatzl/8152438/webrev.1 (full)
> http://cr.openjdk.java.net/~tschatzl/8152438/webrev.0_to_1 (diff)

Looks good, Reviwed.

Thanks,
Erik

> Thanks,
>   Thomas