RFR: 8220465: Use shadow regions for faster ParallelGC full GCs
Haoyu Li
leihouyju at gmail.com
Wed Nov 6 07:17:47 UTC 2019
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.
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.
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> 于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>:
> >
> > 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-region-incr.patch
Type: text/x-patch
Size: 9843 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20191106/dacdf7d9/shadow-region-incr.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shadow-region-v3.patch
Type: text/x-patch
Size: 28686 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20191106/dacdf7d9/shadow-region-v3.patch>
More information about the hotspot-gc-dev
mailing list