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

Kim Barrett kim.barrett at oracle.com
Tue Jan 6 00:03:39 UTC 2015

On Jan 1, 2015, at 9:07 AM, Erik Österlund <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.)

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

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

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

> 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

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

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.

> 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

More information about the hotspot-dev mailing list