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

[Thomas Schatzl]

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