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

[Erik Österlund]

Hi Kim,

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:

On Jan 1, 2015, at 9:07 AM, Erik Österlund <erik.osterlund at lnu.se<mailto:erik.osterlund at lnu.se>> wrote:

Personally I don’t see how the number of rows of code define uglyness. Reusability, testing, documentation and well definedness in contracts/behaviours all lead to more lines of code, but I don’t think that equals uglyness. So while I disagree more code is uglier because it’s more, it’s all up to opinion and it’s quite pointless to discuss; you are entitled to think that is ugly.

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.

:)


While I disagree it is universally incorrect (as long as it is well defined and tested) to not have the exact same requirement on completeness on the parameters as C++11/boost, who also already have different behaviour in this exact regard as pointed out by yourself, I can try to fix it if it bugs you, but it will make the infrastructure even larger which seems to be a concern.

[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/


It could make sense to have a bespoke predicate for determining
whether a conditionally defined class is derived from some known base,
with a usage requirement that if not defined (e.g. complete) in the
current translation unit that it will not be defined anywhere else in
the program. However, I think such a predicate should have a more
specific / informative name and/or shouldn't be classified as a
general utility trait.

But I think even renaming or moving it wouldn't be right, since I
disagree with the need for such a predicate at all.  [More below.]

The main idea of SelectBaseClass is to inject an optional class into the class hierarchy for specialization of generalized behaviour. It is absolutely critical (and intended) that this class is constrained to be a derived class of the fall-back class - it is not an artifact nor an accidental behaviour. In the hierarchy A is-maybe-a B is-a C, A must be able to rely on the superclass being at least C-like so that the behaviour of the superclass can be used reliably and transparently regardless of the existence of the optional B. If this contract would not hold, I think it could lead to some really weird and unreliable code which should not be encouraged (at least not according to the Liskov substitution principle etc).

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.


So what I need in the contract is a check if the optional class is defined and if it fits in the inheritance hierarchy: they are both important IMO.

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. :)


Yes and that makes me wonder if we care enough about the completeness semantics being the same as third party libraries (who are already inconsistent) if we do not need them to be, and on the contrary benefit from them not to be.

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.


In the end, we have to make a choice - what is more important, that the traits resemble some external library or code size. […]

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. ;)


1. Do we want to replace the macros with template metaprogramming? 2 reviewers (and me) liked the metaprogramming approach and it was implemented because they (like me) did not want the macros. But you have a different opinion. I can’t judge who is right and wrong here.

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