8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space

[White, Derek]

Hi Michihiro,

FYI, this patch does seem to help AArch64 also on SPECjbb to a lesser degree. This was benchmarked with very large young gen, so GC overhead is kept lower than you’d see in typical applications.


  *   Derek

From: hotspot-gc-dev [mailto:hotspot-gc-dev-bounces at openjdk.java.net] On Behalf Of Michihiro Horie
Sent: Wednesday, July 04, 2018 4:26 AM
To: Kim Barrett <kim.barrett at oracle.com>
Cc: hotspot-gc-dev at openjdk.java.net; Gustavo Romero <gromero at linux.vnet.ibm.com>
Subject: Re: 8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space


External Email

Hi Martin, Kim,

Thank you for both of your comments.

I missed the point that oopDesc::forward_to is invoked from several callers. Using OrderAccess:storestore() before the invocation of forward_to() would be a great idea, thanks.

>I haven't looked carefully at the change, though I did find one part

>that I don't like. The new test of "order" in forward_to_atomic not

>only affects CMS, but also (uselessly) affects G1.
Please let me confirm your point. You mean I should give memory_order_acq_rel to forward_to_atomic, which uses tests as follows to hold the consistent meaning of acquire/release in forward_to_atomic? I agree it is not clear the test with release returns the forwardee with acquire.

oop oopDesc::forward_to_atomic(oop p, atomic_memory_order order) {
:
while (!oldMark->is_marked()) {
if (order == memory_order_acq_rel) {
curMark = cas_set_mark_raw(forwardPtrMark, oldMark, memory_order_release);
} else {
curMark = cas_set_mark_raw(forwardPtrMark, oldMark, order);
}
}
:
}
if (order == memory_order_acq_rel) {
return forwardee_acquire();
}
return forwardee();
}


Best regards,
--
Michihiro,
IBM Research - Tokyo

[Inactive hide details for Kim Barrett ---2018/07/04 05:41:02---> On Jul 3, 2018, at 4:25 AM, Michihiro Horie <HORIE at jp.ibm.com>]Kim Barrett ---2018/07/04 05:41:02---> On Jul 3, 2018, at 4:25 AM, Michihiro Horie <HORIE at jp.ibm.com<mailto:HORIE at jp.ibm.com>> wrote: >

From: Kim Barrett <kim.barrett at oracle.com<mailto:kim.barrett at oracle.com>>
To: Michihiro Horie <HORIE at jp.ibm.com<mailto:HORIE at jp.ibm.com>>
Cc: "Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>, "hotspot-gc-dev at openjdk.java.net<mailto:hotspot-gc-dev at openjdk.java.net>" <hotspot-gc-dev at openjdk.java.net<mailto:hotspot-gc-dev at openjdk.java.net>>, Gustavo Romero <gromero at linux.vnet.ibm.com<mailto:gromero at linux.vnet.ibm.com>>
Date: 2018/07/04 05:41
Subject: Re: 8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space

________________________________



> On Jul 3, 2018, at 4:25 AM, Michihiro Horie <HORIE at jp.ibm.com<mailto:HORIE at jp.ibm.com>> wrote:
>
> Hi Martin,
>
> Thanks a lot for your review. Sure, we need an OK from a CMS expert. Following is the new webrev:
> http://cr.openjdk.java.net/~mhorie/8205908/webrev.01/
>
> >Seems like a user of the forwardee needs to rely on memory_order_consume in the current implementation. I guess it will be appreciated that you’re fixing this.
> Thank you for pointing out this issue in the original implementation. I newly inserted a release at "2.4. Set new_obj as forwardee [L1142]".
>
> Improvement of critical-jOPS in SPECjbb2015 was 10%, which is still a big number.
>
>
> Best regards,
> --
> Michihiro,
> IBM Research - Tokyo

CMS was deprecated in JDK 9, and has been on maintenance life-support
for some time.  This complex-to-review performance enhancement was
proposed less than 48 hours before JDK 11 FC, and didn't receive any
reviews until after FC.  Because of these factors, I don't think it
should be included in JDK 11. And if CMS gets removed in JDK 12 (I
don't know if that will happen), then this change would be rendered
entirely moot.

I haven't looked carefully at the change, though I did find one part
that I don't like.  The new test of "order" in forward_to_atomic not
only affects CMS, but also (uselessly) affects G1.

I'm not going to be able to look at this carefully soon, as JDK 11 bug
fixing has a higher priority for me.  Since I think CMS might soon not
be an issue, I'd really rather not look at it at all.  I think this
change needs not just a CMS-expert reviewer, but someone who is
willing to maintain CMS (including any potential bug tail from this
change).





-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180709/fb752f86/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 105 bytes
Desc: image001.gif
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180709/fb752f86/image001.gif>