RFR(M): 8154736: enhancement of cmpxchg and copy_to_survivor for ppc64

[Doerr, Martin]

Hi Hiroshi,

when looking over the change for the first time, I had missed that the cmpxchg_post_membar is not safe for future enhancements:
Please use the condition "else if (order != memory_order_relaxed)" for the sync as in cmpxchg_pre_membar. The code should still work reliably if somebody adds new enum values.
I think this is a key property to justify the safety of this change. Adding enum values should not break any platform. This can be established by using maximum conservative barriers for unknown values.

If I remember correctly, some reviewers had complained about the acquire barriers. I think it will be better to present the change without them as this minimizes the impact to Oracle platforms. At least the comment "call acquire for reading fields of new_obj in callers" does not apply to any supported platform (Alpha is not supported) and should be changed.
I think a precise specification of the required ordering semantics is important. This is the second part which is needed to justify the safety of this change.

Best regards,
Martin


From: Hiroshi H Horii [mailto:HORII at jp.ibm.com]
Sent: Samstag, 29. Oktober 2016 12:37
To: Thomas Schatzl <thomas.schatzl at oracle.com>
Cc: David Holmes <david.holmes at oracle.com>; hotspot-compiler-dev <hotspot-compiler-dev at openjdk.java.net>; hotspot-gc-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net; Kim Barrett <kim.barrett at oracle.com>; Doerr, Martin <martin.doerr at sap.com>; ppc-aix-port-dev at openjdk.java.net
Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and copy_to_survivor for ppc64

Hi Thomas,

>   we in the gc team have discussed this change quite a bit internally.
> Overall, we think this change seems far too risky from both a
> functional and performance perspective to go into 9 at this time.

Thank you for your comments and giving a decision.
I completely agree with the decision and would like to keep contributing
to this change for future releases.

> We also think the testing needs to include both functional and
> performance testing, and the performance testing ought to be using some
> well-chosen benchmarks. (It was pointed out very early in the
> discussion of this change that specjbb2013 is deprecated, yet that is
> the only benchmark that's been reported out.)

I see. I will try other workloads and evaluate effects of this change.

> The most recent change also penalizes current platforms that do not
> implement the release-CAS with an additional acquire. That might be not
> an issue for TSO platforms, but others will be affected.
>
> While we think other platforms could quickly adapt to this, this would
> force that the developer that implements this for other platforms
> (arm/aarch64) to be stuck with re-analyzing these issues. We
> do not think this is fair. We think this is a change (or set of
> changes) that needs to be pushed for all platforms at the same time.

Sure. I would like to ask developers for the other platforms to consider
this change.

> There also one (minor) question about the change: why isn't the CAS
> result value being used for the failing paths of the CAS, rather than
> reloaded in copy_to_survivor_space?

I believe, the original code also doesn't use the CAS result because
the current cas_forward_to doesn't return the CAS result value.

bool oopDesc::cas_forward_to(oop p, markOop compare, cmpxchg_memory_order order)

I guess, reloading a forwardee is not expensive because CAS fails are rare,
then maintenanceability was emphasized.

"Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote on 10/21/2016 21:57:42:
> The webrev also contains a logging change in
> psPromotionManager.inline.hpp which I'm not sure if it's still wanted.

For the future discussion, I would like to inform a webrev that doesn't
have any changes of log formats.
http://cr.openjdk.java.net/~horii/8154736/webrev.06/

Regards,
Hiroshi
-----------------------
Hiroshi Horii, Ph.D.
IBM Research - Tokyo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20161031/e6e05185/attachment.htm>