RFR: 8067790: Better support for native implementations of Atomic::cmpxchg

[Erik Österlund]

Hi Kim,

[This is a second identical email from before, hoping nesting levels of comments do not get flattened this time.]

Thank you for your detailed feedback.

On 6 jan 2015, at 00:03, Kim Barrett <kim.barrett at oracle.com<mailto:kim.barrett at oracle.com>> wrote:

Code surface area has an impact on understanding and maintenance.
It's not the only factor, but it *is* a factor.

That said, I looked more closely and found the webrev new line count
substantially overstates the real size of the proposed change. (Nearly
half of the new lines are copyright headers and similar boiler plate.
A substantial fraction of the remainder are tests, which I wouldn't
count as a maintenance negative.) So rather than 800 lines, its
probably more like 100 new lines. That still seems to me to be a lot
in order to eliminate a few flag macro definitions and the
associated #ifdef, but seems much more plausible to amortize across
multiple uses. Since I think between us we've come up with several
more possible use-cases, I'm willing to withdraw the size complaint.

:)

[The difference between boost and C++11 is_base_of is minor, with
boost being more restrictive than necessary. I fully expect someone to
eventually file a bug report about that difference and for the boost
trait to be brought into conformance with the C++11 version.  I think
that difference isn't relevant to this discussion.]

It is not correct as a general utility trait because without the
requirement that "Derived" be complete one can very easily introduce
an ODR violation. That's the rationale for the C++11 requirement. And
it's better to check for that possibility, if possible, than to
silently risk an unchecked ODR violation that could be challenging to
track down. (C++11 doesn't require a diagnostic, leaving that as a QoI
issue. g++ and boost issue a diagnostic for this; I haven't tested
other compilers.)

Point taken, and in my latest proposal including IsBaseOf, a compiler error is generated if Derived is incomplete to comply with your request. Instead I use your class IsClassDefined first to determine if it exists and then IsBaseOf to enforce a stronger contract. Thanks for that contribution. The result can be read in  http://cr.openjdk.java.net/~jwilhelm/8067790/webrev.02/

In the proposed change and in most of the other use-cases I've thought
about, no class-based inheritance relationship between the optionally
defined class and the core "base" is required; duck type compatibility
is sufficient, and I think often preferable.

In the specific proposed change, I'm pretty sure there is nothing that
requires any class-based inheritance relationship at all among Atomic,
AtomicBase, or any AtomicPlatform class. Atomic implements most of the
API, and delegates one piece to the selected "AtomicSuper" by explicit
name qualification. If there were no inheritance relationship at all
among those classes, nobody would notice (assuming the selection
process were changed to not require such a relationship).

If AtomicBase and AtomicPlatform were changed as I suggested in one of
my earlier comments, to provide a protected cmpxchg_byte function with
an unqualified call from Atomic, then Atomic would need to be directly
derived from either AtomicBase or AtomicPlatform, but there would be
no need for any inheritance relationship between AtomicBase or
AtomicPlatform. The duck type compatibility of providing an
appropriate cmpxchg_byte function is sufficient. (With such an
approach I would also consider making the selected base for Atomic be
private, since the inheritance is for implementation convenience and
there's no external client that cares about that relationship.)

It is true that in our case nothing would break if the 3 Atomic classes were not explicitly related by inheritance, but implicitly related with duck typing.

However I want to give the user of SelectBaseClass a stronger contract that the base class can be used reliably and must behave consistently because I think that's better than loose implicit promises. I was never a big fan of duck typing.

I think there are use-cases where appropriate inheritance placement of
the optionally defined class is important, but as discussed above, I
don't think this is one of those. Nor are most of the others that I've
thought of.  I don't want to have to introduce unnecessary inheritance
in order to use the new infrastructure.

With all the template infrastructure provided with my max reuse variant, it would be trivial (~5 rows) to make a variant of SelectBaseClass that does not constrain inheritance in case somebody would need it.

The metafunctions have pretty good synergies. And just to make it clear, I know for a fact I will need IsBaseOf (and some of the other metafunctions) anyway to implement automatic closure specialization (e.g. to distinguish between ExtendedOopClosure and OopClosure). So if we do not introduce them now I will still have to introduce them later on.

In those cases where an inheritance relationship really *is* needed, I
think the right approach is to use conditional existence for selection
and then static assert the needed base/derived relationship(s). For
that we would need an IsBaseAndDerived or IsBaseOf, but their use
would be appropriately conditionalized on the existence of the
optional class (possibly only implicitly, e.g. if Atomic needed to be
derived from AtomicBase then we would assert that, and not care
whether it was direct derivation or via AtomicPlatform).

I do not think it is better to leave correctness detection that could be done automatically to be done manually by the programmer for every use of the function.

This seems to me equivalent to saying it is better to use duck typing and write unit tests to detect type errors to be less constrained by a type system. Some may argue this is true but I like my automatic type checks and stronger contracts. But then again such arguing could go on forever without reaching a conclusion. :)

I strongly care about semantics and risk of error for something that
is billed as a general purpose utility. Risk of ODR violations is not
something I would treat lightly. And minor maintainance changes could
easily lead to such, if this proposed IsBaseOf were used as a general
utility.

Point taken, and IsBaseOf is fixed now as you wanted it.

I think this badly mis-characterizes my position. I think the trait
semantics are critial, and I think the proposed traits have a serious
defect that render them inappropriate as general utilities. In
addition, I don't think they are what is needed for the more
specialized technique exemplared by the proposed change and similar
use-cases. Code size doesn't enter into these concerns at all.

With the IsBaseOf completeness now being enforced, and code size being out of the window, the last concern is you may want a SelectBaseClass with weaker contracts not enforcing inheritance at some point in the future, when inheritance for some reason must be disjoint and only duck typing can be relied upon, yes?

In that case I'm sure with the current template infrastructure, it will be easy to add those 5 lines needed to make such a metafunction then. :)

The case where code size matters to me in this discussion is that
we're talking about adding some non-trivial infrastructure to
eliminate some simple macro usage. If we can't amortize that
infrastructure across some reasonable number of use-cases, then I
wouldn't want to add it. I think there probably are sufficient
use-cases to make some additional infrastructure worth-while, but I
think what was proposed is not the infrastructure we need.

I know I am going to need IsBaseOf for automatic closure specialization to detect differences between ExtendedOopClosure and OopClosure. I don't like manually specializing closures when it could be done automatically. But for that I will need this template infrastructure, so I can guarantee they will come in handy and be used more. ;)

I think this is only beneficial if doing so ultimately makes this code
follow a common pattern used elsewhere in our code base. (I say
"ultimately" because this would be the first occurrence of such a
pattern.)

I will provide automatic closure specialization using this template infrastructure (given somebody is willing to run jprt for me hehe).

I hope we are converging a bit now with opinions. With the IsBaseOf implementation fixed the way you want it, code size being toned down as an issue and reuse of template infrastructure promised (for automatic closure specialization), I think that leaves us to a final issue: Stronger contract enforcing inheritance in SelectBaseClass or weaker relying on duck typing. I would like to repeat that with the reusability of the "max reuse" proposal, such a relaxed duck typed conditional class selector metafunction could be implemented when/if needed later with ~5 LoC and I personally do not think it is much to argue about, especially considering IsBaseOf will certainly be needed soon anyway.

Thanks,
/Erik