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

[Thomas Schatzl]

Hi,

On Fri, 2016-10-07 at 13:23 +1000, David Holmes wrote:
> On 7/10/2016 12:50 PM, Hiroshi H Horii wrote:
> > 
> > Dear Kim, David, and all,
> > 
> > Thank you for your comments.
> > 
> > I created a new webrev. I added memory_order_release as a new enum
> > of
> > cmpxchg_memory_order (atomic.hpp) and use it to update forwardees.
> > 
> > http://cr.openjdk.java.net/~horii/8154736/webrev.04/
> I think you intended to modify cmpxchg_pre_membar not 
> cmpxchg_post_membar! Release semantics require the "post" fence.
> Though technically release semantics would put the barrier before the
> store, not after. But with no pre-fence you could in theory have a
> store before the cas move inside the cas implementation (on ppc/arm)
> and get reordered with the store performed by the cas.
> 
> src/share/vm/gc/parallel/psPromotionManager.cpp still uses 
> memory_order_relaxed.
> 
> That aside this seems too reactive to me. Kim may be right that
> release semantics are sufficient for this code, but that is a claim
> that needs some consideration and validation before we just run with
> it and make the change. The approach to changes like this needs a lot
> more discipline and methodology in my opinion.

There are some other small issues with the suggested change:

- the idiom used to print trace log messages

 244       if (log_develop_is_enabled(Trace, gc, scavenge)) {
 245         log_develop_trace(gc, scavenge)("{%s %s " PTR_FORMAT " ->
" PTR_FORMAT " (%d)}",

does not require the first line.

Log_develop_trace() will only generate code when compiled in debug mode anyway, so the check before that is superfluous.

I saw that in several places.

- could you explain what the advantage of

 298   if (!o->is_forwarded()) {
 299     copy_to_survivor_space<promote_immediately>(o);
 300   }
 301   oop new_obj = o->forwardee();

compared to

 281   oop new_obj = o->is_forwarded()
 282         ? o->forwardee()
 283         : copy_to_survivor_space<promote_immediately>(o);

in PSPromotionManager::copy_and_push_safe_barrier() is?

This seems to introduce a superfluous forced reload (forwardee()
accesses a volatile variable), as copy_to_survivor_space already
reloads and returns the forwardee even with all these changes.

I may be overlooking something crucial (and it's Friday), but I do not see a difference in behavior (and problems) compared to old code, just the additional load.

- the new assert at

 302   assert(forwardee != NULL, "forwardee should not be NULL");

seems superfluous. At this point, after the CAS has been executed, we
assume that there must be a forwardee. Either copy_or_survivor_space
returns it, or there has already been a forwardee.

Thanks,
  Thomas