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

[Michihiro Horie]

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.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20180519/a8ef8b0a/attachment.html>