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

Wed Jan 7 03:33:11 UTC 2015

src/os_cpu/linux_x86/vm/atomic_linux_x86.inline.hpp

-inline void*    Atomic::xchg_ptr(void*    exchange_value, volatile void*     dest) {
+inline void*    Atomic::xchg_ptr       (void*    exchange_value, volatile void*     dest) {
    return (void*)xchg_ptr((intptr_t)exchange_value, (volatile intptr_t*)dest);
  }

This formatting is weird in this file.  Why is this like this?

Are these os_cpu files essentially the same?

*src/os_cpu/bsd_x86/vm/atomic_bsd_x86.hpp

*

Traditionally we have one declaration of the class AtomicPlatform in a 
common directory and provide the implementations in their platform 
dependent directories.  There shouldn't be duplicate declarations.

http://cr.openjdk.java.net/~jwilhelm/8067790/webrev.02/src/share/vm/utilities/traits/isBaseOf.cpp.html

Is this an internal VM test?  It should be #ifndef PRODUCT if so. Same 
for the other .cpp files.

http://cr.openjdk.java.net/~jwilhelm/8067790/webrev.02/src/os_cpu/windows_x86/vm/atomic_windows_x86.inline.hpp.udiff.html

The parameter spacing is messed up in this file too.  Please check all 
the files to correct spacing.  The existing code has some spacing issues 
also but new code should be correctly formatted.

So now I see why there are duplicate AtomicPlatforms.  The whole purpose 
is that some platforms have atomic::cmpxchg and some don't. Sorry I'm 
late to this thread.

I have to admit that I'm biased by Kim's opinion and I know far less 
about these template tricks than he does.  I don't want to review or 
read all this code in order to avoid some platform specific macros.   Is 
there currently a bug that this code is fixing?  Does this code actually 
add an optimized version of atomic::cmpxchg(byte)?

Coleen

On 1/6/15, 5:37 AM, Erik Österlund wrote:
> 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