RFR: 8220465: Use shadow regions for faster ParallelGC full GCs
thomas.schatzl at oracle.com
Tue Dec 3 10:39:05 UTC 2019
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
> 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:
More information about the hotspot-gc-dev