RFR: 8220465: Use shadow regions for faster ParallelGC full GCs
Haoyu Li
leihouyju at gmail.com
Tue Nov 12 15:11:36 UTC 2019
Hi Stefan,
Thanks for your advice!
Nice, I think it would make sense to used cmpxchg in mark_normal() as
> well and assert that the returned value is SHADOW.
I've changed mark_normal() to use Atomic::cmpxchg and added an assertion.
Please find the changes in the attached patchs. Thanks!
Best Regards,
Haoyu Li,
Stefan Johansson <stefan.johansson at oracle.com> 于2019年11月11日周一 下午11:10写道：
> 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
> >
>
More information about the hotspot-gc-dev
mailing list