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

[Haoyu Li]

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.

- psParallelCompact.cpp: line 3064: RegionSize is now unused, the line
> can be deleted.


I have deleted this line of dead code.


> - 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
shadow regions. Suppose that region A's destination is region B, and thread
T1 is processing region A with the only shadow region C first. If thread T2
then wants to process B with a shadow region, T2 will block in the
pop_shadow_region_mt_safe since _shadow_region_array is empty. However, T1
cannot push C to _shadow_region_array because T1 hasn't copied C back to A,
and T1 actually cannot copy C to A unless T2 has processed B. In such case,
T1 won't notify T2, and therefore T2 has to wake up in a given time period
to check if it should exist the shadow region protocal (return with ~0).
However, this should be a rare case (so I reorder the two if clauses in
ParCompactionManager::pop_shadow_region_mt_safe), and I set the time to 1
ms.

Thanks very much for all your reviewing effort. Look forward to hearing
from you.

Best Regards,
Haoyu Li


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

> Hi,
>
> On 30.11.19 12:52, Haoyu Li wrote:
> > Hi Stefan,
> >
> > Thanks for your reviewing. If there are any further problems in the
> > code, please feel free to contact me! I am always more than happy to
> > improve the code.
>
> Looks good, with the following minor issues:
>
> > > >> - I think there is a missed optimization opportunity in (now)
> > > >> PSParallelCompact::initialize_shadow_regions(). There, the code
> > > >> initializes the "free" region ids to region_at_top+1 to
> > > >> end_region of a particular space.
> > > >>
> > > >> If the top for a given space is at a region boundary (e.g. if
> > > >> a space is empty, which is probably common for one of the
> > > >> survivor spaces), you loose a single region per space.
> > > >>
> > > >> One reason might that the code uses region "0" as sentinel to
> > > >> indicate "there is no shadow region available" in
> > > >> ParCompactionManager::acquire_shadow_region().
> > > >>
> > > >> This could be fixed by improving the code in
> > > >> PSParallelCompact::initialize_shadow_regions() and use a sentinel
> > > >> region value of (size_t)~0 (as an explicit constant).
> > > >>
> > > >> Even if you do not change this, please introduce an explicit
> > > >> onstant for this sentinel value. This makes the code more self-
> > > >> explanatory.
> > > >>
> > > >> Sorry for the misleading +1 operation. The +1 can be safely
> > > >> removed. The sentinel value 0 does not cause this design because
> > > >> the first region (in old space) cannot be a shadow region.
>
> 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.
>
> - psParallelCompact.cpp: line 3064: RegionSize is now unused, the line
> can be deleted.
>
> - 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,
>    Thomas
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-region-incr.patch
Type: text/x-patch
Size: 3964 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20191203/ddb4b0a3/shadow-region-incr.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-region-v7.patch
Type: text/x-patch
Size: 30489 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20191203/ddb4b0a3/shadow-region-v7.patch>