RFR: 8220465: Use shadow regions for faster ParallelGC full GCs
Haoyu Li
leihouyju at gmail.com
Sat Nov 16 05:13:42 UTC 2019
Hi Stefan,
Thanks very much for all your reviewing effort! Please feel free to contact
me if there is any problem. Looking forward to hearing from you!
Best Regards,
Haoyu Li
Stefan Johansson <stefan.johansson at oracle.com> 于2019年11月15日周五 下午5:21写道：
> Hi Haoyu,
>
> Thanks for the updates, here are new webrevs:
> Full: http://cr.openjdk.java.net/~sjohanss/8220465/03/
> Inc: http://cr.openjdk.java.net/~sjohanss/8220465/02-03/
>
> I have asked more people to look at the change now, so you might await
> some more feedback.
>
> Thanks,
> Stefan
>
> On 2019-11-12 16:11, Haoyu Li wrote:
> > 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
> > <mailto: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>
> > > <mailto: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>
> > <mailto: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