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

[David Holmes]

Hi Michihiro,

An initial question ...

Can you clarify the assumptions/expectations with regard to  
Release-Consume given it has been deprecated by the C++ committee?  
memory_order_release is expected to be C++ compatible.

Thanks,
David

On 19/05/2018 1:12 AM, Michihiro Horie wrote:
> Dear all,
> 
> I update the webrev: http://cr.openjdk.java.net/~mhorie/8154736/webrev.09/
> 
> With the release barrier before the CAS, new_obj can be observed from 
> other threads. If the CAS succeeds, the current thread can use new_obj 
> without barriers. If the CAS fails, "o->forwardee()" is ordered with 
> respect to CAS by accessing the same memory location "_mark", so no 
> barriers needed. The order of (1) access to the forwardee and (2) access 
> to forwardee's fields is preserved due to Release-Consume ordering on 
> supported platforms. (The ordering between "new_obj = o->forwardee();" 
> and logging or other usages is not changed.)
> 
> Regarding the maintainability, the requirement is 
> CAS(memory_order_release) as Release-Consume to be consistent with 
> C++11. This requirement is necessary when a weaker platform like DEC 
> Alpha is to be supported. On currently supported platforms, code change 
> can be safe if the code meats this requirement (and the order of (1) 
> access to the forwardee and (2) access to forwardee's fields is the 
> natural way of coding).
> 
> 
> oop PSPromotionManager::copy_to_survivor_space(oop o) {
> oop new_obj = NULL;
> markOop test_mark = o->mark_raw();
> 
> if (!test_mark->is_marked()) { // The same test as "o->is_forwarded()"
> :
> Copy::aligned_disjoint_words((HeapWord*)o, (HeapWord*)new_obj, ...);
> 
> if (o->cas_forward_to(new_obj, test_mark)) { // CAS succeeds
> :
> new_obj->push_contents(this);
> 
> } else { // CAS fails
> :
> new_obj = o->forwardee();
> }
> } else {
> :
> new_obj = o->forwardee();
> }
> 
> log_develop_trace(gc, scavenge)(..., new_obj->klass()->internal_name(), 
> p2i((void *)o), p2i((void *)new_obj), new_obj->size());
> return new_obj;
> }
> 
> 
> Best regards,
> --
> Michihiro,
> IBM Research - Tokyo
> 
> 
> ----- Original message -----
> From: Kim Barrett <kim.barrett at oracle.com>
> To: David Holmes <david.holmes at oracle.com>
> Cc: Michihiro Horie <HORIE at jp.ibm.com>, 
> ppc-aix-port-dev at openjdk.java.net, hotspot-dev at openjdk.java.net, 
> hotspot-gc-dev at openjdk.java.net, Hiroshi H Horii <HORII at jp.ibm.com>
> Subject: Re: RFR(M): 8154736: enhancement of cmpxchg and 
> copy_to_survivor for ppc64
> Date: Fri, Apr 27, 2018 6:03 AM
> 
>  > On Apr 25, 2018, at 8:45 AM, David Holmes <david.holmes at oracle.com> 
> wrote:
>  >
>  > Hi Michihiro,
>  >
>  > On 23/04/2018 8:33 PM, Michihiro Horie wrote:
>  >> Dear all,
>  >> I would like to ask reviews on 8154736 “enhancement of cmpxchg and
>  >> copy_to_survivor”. The change adds options to avoid expensive syncs with
>  >> compare-and-exchange. An experiment by using SPECjbb2015 showed 6%
>  >> improvement in critical-jOPS. This change focuses on ppc64 but would be
>  >> potentially beneficial for aarch64.
>  >> Although discussions stopped at
>  >> 
> _http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2016-October/002718.html_
>  >> , I would like to restart the review by taking over Hiroshi's work 
> if the
>  >> discussion is still open.
>  >
>  > So the very last comment there was about not implicitly assuming 
> memory_order_consume, yet that has not been addressed in the proposal.
>  >
>  > Further the discussion on hotspot-runtime-dev through September and 
> October was far more illuminating. I think my post here:
>  >
>  > 
> _http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-October/021617.html_
>  >
>  > and the closely following one from Thomas Schatzl summed up the 
> concerns about the proposed changes.
>  >
>  > AFAICS the restarted proposal addresses none of those concerns but 
> simply takes up where the previous implementation suggestion left off.
>  >
>  > This is a proposal to change the memory ordering semantics of part of 
> the shared GC code _not_ just the PPC64 implementation, but I have seen 
> no analysis to demonstrate the correctness of such a proposal.
> 
> I agree with David here. So far we've seen no such analysis. All we
> have seen is a series of proposed changes and non-failing test
> results, all of which have then been shown to have holes. (Among other
> things, this suggests the set of tests being applied is inadequate.)
> Part of the author's job is to convince reviewers that the proposed
> change is correct. I'm not expecting a formal proof, but I am
> expecting a lot more than has been provided so far.
> 
> In this latest proposal, the conditional acquire doesn't look right to
> me. If the logging is disabled so there is no acquire, the object is
> then returned to the callers, who might do what with it? Is that safe?
> For all callers? And is it stable through future maintenance? This is
> not to say that I think making those acquires unconditional is
> sufficient.
> 
>