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

[David Holmes]

On 6/01/2015 10:47 PM, Erik Österlund wrote:
> Hi Kim,
>
> [This is a second identical email from before, hoping nesting levels of comments do not get flattened this time.]

Still flat. :(

David

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