RFR: 8220465: Use shadow regions for faster ParallelGC full GCs
Stefan Johansson
stefan.johansson at oracle.com
Fri Nov 15 09:21:50 UTC 2019
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