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

[Haoyu Li]

Hi Stefan,

Thanks for your constructive comments. I will address these issues in the
next few days and provide both a full patch as well as the incremental
changes.

Best Regrads,
Haoyu Li,
Institute of Parallel and Distributed Systems(IPADS),
School of Software,
Shanghai Jiao Tong University


Stefan Johansson <stefan.johansson at oracle.com> 于2019年10月29日周二 上午3:03写道：

> Hi Haoyu,
>
> I’ve looked through the patch in detail now and created a new webrev at:
> http://cr.openjdk.java.net/~sjohanss/8220465/01/
>
> I took the liberty of removing the removal of move_and_update from your
> patch since I’m addressing that separately in JDK-8233065. The webrev above
> is still based on that removal, but I expect that to be pushed tomorrow or
> Wednesday so that should be fine.
>
> I also changed the subject to make it more clear that this is now a review
> of:
> https://bugs.openjdk.java.net/browse/JDK-8220465
>
> Regarding the current patch, I think that it looks good in general, but I
> thought a bit more around how to share stuff between the closures and I
> agree that adding those extra virtual functions doesn’t really feel worth
> it. I’m wondering if a solution where we revert back to letting destination
> be the ”real destination” (not ever pointing to the shadow region) and add
> a copy_destination which is destination + offset. To make this work the
> normal MoveAndUpdateClosure would also have an offset, but it would always
> be 0. If do_addr() is then updated to use the copy_destination() in some
> places we might end up with something pretty nice, but maybe I’m missing
> something.
>
> I also realized that the current patch will trigger an assert because
> destination is expected not to be the shadow address:
> #  Internal Error
> (open/src/hotspot/share/gc/parallel/psParallelCompact.cpp:3045), pid=12649,
> tid=12728
> #  assert(src_cp->destination() == destination) failed: first live obj in
> the space must match the destination
>
> So this also suggests that we should keep destination() returning the real
> destination.
>
> Some other comments:
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> —
> 3383 void ShadowClosure::complete_region(ParCompactionManager *cm,
> HeapWord *dest_addr,
> 3384                                     PSParallelCompact::RegionData
> *region_ptr) {
> 3385   assert(region_ptr->shadow_state() ==
> ParallelCompactData::RegionData::FINISH, "Region should be finished”);
>
> This assertion will also trigger when running with a debug build and at
> this point the shadow state should be SHADOW not FINISH.
> —
>
> src/hotspot/share/gc/parallel/psParallelCompact.hpp
> —
>  632 inline bool ParallelCompactData::RegionData::mark_filled() {
>  633   return Atomic::cmpxchg(FILLED, &_shadow_state, SHADOW) == SHADOW;
>  634 }
>
> Since we never check the return value here we should make it void and
> maybe instead add an assert that the return value is SHADOW.
> —
>
> When you addressed these comments, would it be possible to include both
> the full patch and and the incremental changes from the current version.
> That makes it easier for the reviewers to see what changed between version
> of the patch.
>
> Thanks,
> Stefan
>
> > 24 okt. 2019 kl. 14:16 skrev Stefan Johansson <
> stefan.johansson at oracle.com>:
> >
> > Hi Haoyu,
> >
> > On 2019-10-23 17:15, Haoyu Li wrote:
> >> Hi Stefan,
> >> Thanks for your constructive feedback. I've addressed all the issues
> you mentioned, and the updated patch is attached in this email.
> > Nice, I will look at the patch next week, but I'll shortly answer your
> questions right away.
> >
> >> During refining the patch, I have a couple of questions:
> >> 1) Now the MoveAndUpdateClosure and ShadowClosure assume the
> destination address is the very beginning of a region, instead of an
> arbitrary address like what it used to be. However, there is an unused
> function named PSParallelCompact::move_and_update() uses the
> MoveAndUpdateClosure to process a region from its middle, which conflicts
> with the assumption. I notice that you removed this function in your patch,
> and so did I in the updated patch. Does it matter?
> > Yes, I found this function during my code review and it should be
> removed, but I think that should be handled as a separate issue. We can do
> this removal before this patch goes in.
> >
> >> 2) Using the same do_addr() in MoveAndUpdateClosure and ShadowClosure
> is doable, but it does not reuse all the code neatly. Because storing the
> address of the shadow region in _destination requires extra virtual
> functions to handle allocating blocks in the start_array and setting
> addresses of deferred objects. In particular, allocate_blocks() and
> set_deferred_object_for() in both closures are added. Is it worth avoiding
> to use _offset to calculate the shadow_destination?
> > Ok, sounds like it might be better to have specific do_addr() functions
> then. I'll think some more around this when reviewing the new patch in
> depth.
> >
> >> If there are any problems with this patch, please contact me anytime.
> I'm more than happy to keep improving the code. Thanks again for reviewing.
> >>
> > Sound good, thanks,
> > Stefan
>
>