8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space

[Michihiro Horie]

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



From:	Kim Barrett <kim.barrett at oracle.com>
To:	Michihiro Horie <HORIE at jp.ibm.com>
Cc:	"Doerr, Martin" <martin.doerr at sap.com>,
            "hotspot-gc-dev at openjdk.java.net"
            <hotspot-gc-dev at openjdk.java.net>, Gustavo Romero
            <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> 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/20180704/50d40d90/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180704/50d40d90/graycol.gif>