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

[Stefan Johansson]

Hi Thomas and Haoyu,

> 3 dec. 2019 kl. 11:39 skrev Thomas Schatzl <thomas.schatzl at oracle.com>:
> 
> 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)
> 

All these changes look good to me, great work :)

Thanks,
Stefan

> Thanks,
>  Thomas