RFR: 8220465: Use shadow regions for faster ParallelGC full GCs

[Haoyu Li]

Hi Thomas and Stefan,

Thanks for your quick replies. The changes also look great to me, but I
noticed that a few more statics and assertions could be improved, so I have
updated them in the attached patches.  I could have finished these updates
earlier. However, there was a network issue that I cannot pull the latest
code. Should you have any further suggestions, I am happy to adopt them.

Best Regards,
Haoyu Li


Thomas Schatzl <thomas.schatzl at oracle.com> 于2019年12月3日周二 下午6:39写道：

> Hi,
>
> On 03.12.19 10:08, Haoyu Li wrote:
> > Hi Thomas,
> >
> > Thanks for your reviewing. Attachments are the updated patches, and
> > details are as follows.
> >
> > > I was trying to come up for a reason that "the first region cannot be a
> > > shadow region", could you explain? What about if compaction starts off
> > > with empty old gen? In this case probably the situation can't occur,
> > > but
> > > still it seems more complicated than an obviously invalid shadow region
> > > id (which is far beyond the heap).
> > >
> > > Please change to use the ~0 sentinel (with a properly named constant)
> > > instead - this is much easier to reason about than otherwise.
> >
> >
> > Sorry for the oversimple explanation. The first region cannot be a
> > shadow region because we collect empty regions between max(top, new_top)
> > and end of each space as shadow regions. For old space, though it could
> > be empty, its new_top would be greater than 0 unless other spaces are
> > also empty, which I think is impossible during a full GC. Therefore,
> > max(top, new_top) will be greater than 0, and we won't collect region 0
> > as a shadow region.
> >
> > But I agree with you that using the ~0 sentinel is much easier to reason
> > about the case. I have replaced the 0 with ~0
> > (ParCompactionManager::InvalidShadow) in the patches to avoid the above
> > implicit logic.
>
> Thanks, looks good.
>
> > > - I'd prefer if the push/pop_shadow_region_mt_safe methods used a
> > > MonitorLocker and a wait/notify scheme for synchronization. The pop
> > > method locking the _shadow_region_monitor in the infinite loop body
> > > (and
> > > not doing anything else) seems problematic when there is contention
> > > (if).
> >
> >
> > Thanks for the helpful suggestion! I have changed the MutexLocker to the
> > MonitorLocker/wait/notify scheme. I use the timed wait because of a
> > possible deadlock situation between two GC threads popping and pushing
>  > [...]
>  > (so I reorder the two if clauses in
>  > ParCompactionManager::pop_shadow_region_mt_safe), and I set the time
>  > to 1 ms.
>
> Stefan and me were just discussing this issue and to use a timed wait
> too. :)
>
> >
> > Thanks very much for all your reviewing effort. Look forward to hearing
> > from you.
>
> Looking at the patch, a few (further) minor nits that I fixed in the
> webrevs further below:
>
> - I rebased the change to latest code, e.g. the parameter order in
> Atomic::cmpxchg changed recently. Basically the first parameter
> (exchange_value) got moved to the third parameter.
> - we prefer to access statics via the class name, i.e.
> ParCompactionManager::InvalidShadow instead of cm->InvalidShadow.
> - some asserts were not updated to latest names of the states
>
> Please take a look if these changes are correct. I started a few
> internal regression testing runs.
>
> First, your recent changes as webrevs:
> http://cr.openjdk.java.net/~tschatzl/8220465/webrev.5_to_6/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8220465/webrev.6/ (full)
>
> Above changes:
> http://cr.openjdk.java.net/~tschatzl/8220465/webrev.6_to_7/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8220465/webrev.7/ (full)
>
> Thanks,
>    Thomas
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-region-incr.patch
Type: text/x-patch
Size: 4898 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20191204/8f10499f/shadow-region-incr.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-region-v8.patch
Type: text/x-patch
Size: 30576 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20191204/8f10499f/shadow-region-v8.patch>