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

Mon Nov 11 15:08:10 UTC 2019

Hi Haoyu,

Thanks for the updated patches, I think they look good in general, just 
one comment inline below.

Here are some updated webrev:
Full: http://cr.openjdk.java.net/~sjohanss/8220465/02
Inc: http://cr.openjdk.java.net/~sjohanss/8220465/01-02

On 2019-11-06 08:17, Haoyu Li wrote:
> Hi Stefan,
> 
> Sorry for the late update. I have attached both a full patch 
> (shadow-region-v3.patch) and the incremental changes 
> (shadow-region-incr.patch) in this mail, and details are as follows.
> 
>     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.
> 
> 
> It is an excellent idea to let MoveAndUpdateClosure have an _offset 
> equal to 0, so ShadowClosure can reuse more code from it. I have made 
> the above changes in the new patch.
Yes, using this approach looks very nice.

> 
>     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.
>     —
> 
> 
> Sorry for these buggy assertions. The shadow_state in 
> ShadowClosure::complete_region should be SHADOW instead of FINISH, and 
> I've corrected it. Moreover, while I was testing it in the debug mode, I 
> found another interesting case, in which a region should return to the 
> normal path if it becomes available before invoking fill_shadow_region 
> (the branch that shadow_region == 0 at psParallelCompact.cpp:3182). 
> Therefore, I add a new function 
> ParallelCompactData::RegionData::mark_normal() to handle this special 
> case, so the assertion in MoveAndUpdateClosure::complete_region will 
> success.
Nice, I think it would make sense to used cmpxchg in mark_normal() as 
well and assert that the returned value is SHADOW.

Thanks,
Stefan

> 
>     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.
>     —
> 
> 
> Thanks for the suggestion. I have changed mark_filled() to void.
> 
> I really appreciate your reviews. If there are any issues in the patch, 
> please let me know at any time. Thanks again!
> Best Regards,
> Haoyu Li
> 
> Stefan Johansson <stefan.johansson at oracle.com 
> <mailto: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 <mailto: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
> 