RFR: 7143664: Clean up OrderAccess implementations and usage

[Kim Barrett]

I’m breaking up some of the issue discussions into separate emails, mostly
so that I don’t feel like I need to address all of them before sending any reply.

On Feb 20, 2015, at 8:08 PM, Erik Österlund <erik.osterlund at lnu.se> wrote:
> 
> On 20/02/15 23:41, Kim Barrett wrote:
>>> http://cr.openjdk.java.net/~dholmes/7143664/webrev.v4/
>> 

> src/share/vm/runtime/orderAccess.hpp
> 
> I don't see the point of having ScopedFence derive from
> ScopedFenceGeneral.  It seems to me that ScopedFenceGeneral could
> derive from AllStatic (with corresponding changes to functions) and
> ScopedFence could instead derive from StackObj.
> 
>> It does have a point to me. The reason is simple: ScopedFence is-a 
>> ScopedFenceGeneral - and that's the point. Whether the inheritance is 
>> technically needed or could be removed is IMO not important. Because 
>> it's more intuitive for my understanding that if it's used like 
>> inheritance and ScopedFence is-a ScopedFenceGeneral, then they should be 
>> related by inheritance whether required for the code to compile or not.
>> 
>> It's like AllStatic classes with inheritance: they might not technically 
>> need the inheritance because calls to super are qualified anyway, but 
>> they should use inheritance anyway if they are conceptually related by 
>> inheritance IMO.
>> 
>> If A is-a B, then A should inherit from B.
>> 
>> Would you agree with this?

No, I don't agree.

I don't see ScopedFence as being is-a ScopedFenceGeneral.  Rather, I
see ScopedFenceGeneral as an implementation strategy for ScopedFence.

No client code should ever deal with ScopedFenceGeneral. It's purely
an implementation detail providing the extra indirection needed for
the default implementation of ScopedFence, while still allowing the
latter to be specialized by a platform.  As such, exposing it as a
public base class is inappropriate.

ScopedFenceGeneral is also poorly written as a base class.  It allows
trivial accidental slicing in various ways.  To prevent that, none of
it's API ought to be public.  This includes not just the explicit
explicit prefix() and postfix(), but also the automatically generated
ctor/dtor/copy/assign operations.  But once it has no public API it
ceases to be interesting as a public base class.

Below is a sketch of how I think ScopedFence should be done.  Note
that the public ScopedFenceGeneral has been replaced with the private
ScopedFence::Impl.

----------
#include <iostream>

enum ScopedFenceType {
    X_ACQUIRE
  , RELEASE_X
  , RELEASE_X_FENCE
};

template<ScopedFenceType T>
class ScopedFence /* : public ScopedObj */ {
  class Impl;

public:
  ScopedFence() { prefix(); }
  ~ScopedFence() { postfix(); }
  void prefix() { Impl::prefix(); }
  void postfix() { Impl::postfix(); }
};

template<ScopedFenceType T>
struct ScopedFence<T>::Impl /* : public AllStatic */ {
  static void prefix() { }
  static void postfix() { }
};

// Default implementations for specific fence types
template<> inline void ScopedFence<X_ACQUIRE>::Impl::postfix() {
  std::cout << "ScopedFence<X_ACQUIRE>::postfix() => Impl\n";
}

template<> inline void ScopedFence<RELEASE_X>::Impl::prefix() {
  std::cout << "ScopedFence<RELEASE_X>::prefix() => Impl\n";
}

// platform-specific override
template<> inline void ScopedFence<X_ACQUIRE>::postfix() {
  std::cout << "ScopedFence<X_ACQUIRE>::postfix() => override\n";
}

int main(int argc, char* argv[]) {
  ScopedFence<X_ACQUIRE> a;
  ScopedFence<RELEASE_X> r;
  return 0;
}