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

[Stefan Johansson]

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