RFR: 7143664: Clean up OrderAccess implementations and usage

David Holmes david.holmes at oracle.com
Sun Feb 22 04:51:46 UTC 2015

My 2c as the sponsor here. If we get into matters of style then we can 
debate endlessly, so lets not do that :) (or at least lets not have that 
hold up getting this pushed through - we can debate and refine later if 
needed). As far as macros are concerned I'd need to see a concrete 
example of what is proposed to evaluate its utility with regard to 
understandability or maintainability - but as a template novice I find 
the current structure quite readable and understandable.

Re Atomics ... seems to be a separate issue. For a while people 
overlooked the fact that 64-bit loads/stores needed special handling on 
32-bit systems. Hence we need specific Atomic handling for jlong and 
jdouble. For all other types (regardless of whether there is an 
Atomic::store variant) we already assume accesses are atomic (which may 
only be true if alignment constraints are met).

Re: > 
> src/os_cpu/linux_ppc/vm/orderAccess_linux_ppc.inline.hpp
> I see no uses of the inlasm_eieio macro.
> I see no uses of the inlasm_isync macro.

This was put there by the ppc64 folk. It is up to them whether it is 
removed or left in place for potential future use.


On 21/02/2015 11:08 AM, Erik Österlund wrote:
> Hi Kim,
> Thanks for the review! :)
> In general: I might be a bit biased against the use of macros in general
> and avoid it whenever it can be avoided as you know. :)
> To me it seems a bit over engineered and unnecessarily complex to reduce
> the already quite small (84 LoC) code size of OrderAccess.inline.hpp
> with macros. And for me it's unclear what the benefit of such code size
> decrease would be. A means to reduce complexity, increase
> understandability? Probably not. A means to increase maintainability?
> Perhaps possible but not sure!
> But in matters where it's really up to personal taste, I'd like to not
> decide. Kim I will let you call the shots!
> Comments:
> On 20/02/15 23:41, Kim Barrett wrote:
>>> http://cr.openjdk.java.net/~dholmes/7143664/webrev.v4/
>> I haven't reviewed all of the changes, instead focusing on the
>> structure and use of templates.  That generally looks fine; I have a
>> few stylistic comments below, but nothing earth shaking.
>> Some of the remaining comments, even if technically correct and
>> acceptable, might not be appropriate for this change set, but should
>> instead be dealt with in follow-on changes.
>> ------------------------------------------------------------------------------
>> Throughout:
>> I would probably have used some simple macros to avoid near
>> duplication of a lot of groups of related functions that differ only
>> in a type parameter.
>> The present approach of making the groups of related functions
>> "obviously" similar by putting the entire definition on one line so
>> there are several nearly identical lines in a row makes me look at
>> each of the lines carefully to make sure the several parallel
>> differences are what's expected.  It has the additional defect of
>> involving very long lines, which makes them harder to review in
>> side-by-side views.
>> YMMV.
> If I get you right, you would like to use macros to generate code lines
> that are now instead aligned but longish, in order to make the lines
> shorter?
> Hmm, it would indeed become shorter lines then. I think it would make it
> harder to read and understand though as a consequence. Do you think it's
> worth macroing it up to reduce code size? I mean the motivation/point of
> reducing code size is often to decrease complexity, but I feel like here
> it would on the contrary induce unnecessary complexity just for the sake
> of having shorter lines. Hmm!
> I don't know, what is the policy in general for sticking to 80
> characters BTW? Personally I was never a fan of this, especially not for
> C++ out of all languages because it sometimes requires long types,
> qualified scopes etc. All lines fit on my screen, so I never considered
> it a problem. I guess it's a matter of taste (and screen size haha).
> Personally I would rank readability in this order (which explains why it
> looks this way now):
> 1. Long aligned lines
> 2. Shorter split but not aligned lines
> 3. Macros
>> ------------------------------------------------------------------------------
>> The ordered access operations are defined on jbyte ... jlong.  The
>> problem is that there is a type in that sequence that is the same size
>> as two distinct C/C++ types.  And as a result, one of those C/C++
>> types is missing atomic support.  This can lead to problems with types
>> like size_t on some platforms
>> I also don't see atomic support for char*.
>> This might be something to consider for a separate change.
> Sorry I don't think I understand what you mean here. :(
> If I get you right, you want more responsibilities to move to Atomic
> regarding making atomic accesses? In that case I agree. :)
>> ------------------------------------------------------------------------------
>> An observation:
>> If OrderAccess was a real namespace instead of a pseudo-namespace
>> class, the specialized versions of the specialized_xxx functions could
>> be ordinary function overloads rather than function template
>> specializations.
>> OTOH, namespaces don't provide private access control.  I often find
>> myself wishing for such a feature.
>> No actual change needed here, just pointing out an interesting
>> variation from the approach taken.
> The neat thing about template specializations that I wanted to exploit
> here is that the definition is the declaration. For overloads, you have
> to somehow declare the function overloads to be specialized somewhere
> (which would be different for every platform as there are different sets
> of specializations). I think it would have been a bit more awkward
> because of that. With the template specializations on the other hand,
> you just stick the definition in there and the compiler will
> automatically know it is available too without any need for a
> declaration of available template specializations.
> I don't think you could do that with the namespace+overload approach, right?
>> ------------------------------------------------------------------------------
>> An observation:
>> load_ptr_acquire isn't strictly necessary, and could be replaced by
>> load_acquire with some additional work.  But it might be the "_ptr_"
>> in the name is considered useful documentation.
>> Given that, load_ptr_acquire can be generalized to support any type of
>> pointer, not just intptr_t* and void*.  This would eliminate the need
>> for casting result types in some places, and for some strange value
>> types in others.  For example MetaspaceGC::_capacity_until_gc is
>> declared intptr_t rather than size_t.
>> This might be something to consider for a separate change.
> Interesting you mention this! I had similar thoughts.
> My analysis about _ptr_:
> The _ptr_ is required because both some kind of pointer type (like
> void*) and also intptr_t was wanted. Now intptr_t will be the same as
> either jint or jlong, which already has overloads. Therefore, this would
> lead to compiler moaning about ambiguity. I have no problem with leaving
> it like that though.
> My analysis about generic pointer types:
> Ideally we would want generic/parameterized pointer types, like:
> release_store(T *volatile *addr, T *value)
> And it looks very strange that it is instead
> release_store(volatile void *addr, void *value)
> The reason it looks like this is because any pointer is implicitly
> casted to void* which has been abused all over the place. So you find
> actual usages like:
> OrderAccess::release_store_ptr((Klass* volatile
> *)obj_at_addr_raw(which), k);
> Here the user gave an exact pointer type. But there is no such overload,
> so it is implicitly casted to void* like any other pointer. The same
> does *not* work for void**, and that's why void* is there even though it
> seems very wrong.
> I tried making a generalized pointer version in pretty much exactly the
> way you suggest using templates. And it didn't work, because since
> everything has been just implicitly casted to void* all over the place
> regardless of actual pointer type, the code is split between sometimes
> giving good exact pointer types and sometimes bad ones not following the
> intended pattern. So while this ideally would be nicer, I gave up on
> that for this change - we need to clean up the actual usage of
> OrderAccess too in order to do this. :(
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.hpp
>> I don't see the point of having ScopedFence derive from
>> ScopedFenceGeneral.  It seems to me that ScopedFenceGeneral could
>> derive from AllStatic (with corresponding changes to functions) and
>> ScopedFence could instead derive from StackObj.
> It does have a point to me. The reason is simple: ScopedFence is-a
> ScopedFenceGeneral - and that's the point. Whether the inheritance is
> technically needed or could be removed is IMO not important. Because
> it's more intuitive for my understanding that if it's used like
> inheritance and ScopedFence is-a ScopedFenceGeneral, then they should be
> related by inheritance whether required for the code to compile or not.
> It's like AllStatic classes with inheritance: they might not technically
> need the inheritance because calls to super are qualified anyway, but
> they should use inheritance anyway if they are conceptually related by
> inheritance IMO.
> If A is-a B, then A should inherit from B.
> Would you agree with this?
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.hpp
>> Why are ScopedFenceType, ScopedFenceGeneral, and ScopedFence defined
>> at global scope rather than nested in OrderAccess?
> I actually had them nested at first. But eh, the lines in definitions
> became pretty long then because of the long nested names and I thought
> I'd make the lines a bit tighter. I think I just contradicted myself
> haha! Anyway, if you prefer them to be nested I'm fine with that: I do
> not feel strongly about this. :)
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.hpp
>> For all load_acquire variants, why (volatile T* p) rather than
>> (const volatile* p)?
> Did you mean (const T * volatile *p)? I assume so. And yes I had the
> same question. And like before, I tried it and ran into a lot of
> compiler errors because of implicit void* casts abused and incompatible
> with void** throughout hotspot, and gave up on it. ;)
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.inline.hpp
>> I'm not sure what the point of this is.  Is this just to allow some
>> (perhaps third-party?) platforms to continue to use an old
>> implementation?
>> Or is it for incremental development of this set of changes?
>> Is the plan that it will eventually go away?
> Yeah it's only here while adapting other platforms and smoothly
> transitioning to the new architecture. Then it will be removed (together
> with the store_fence declarations now made private meanhile).
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.inline.hpp
>> I think there needs to be some documentation here about which
>> specialized_xxx functions can/should be considered for specialization.
> Yes in OrderAccess.inline.hpp I wrote next to the OrderAccess::store
> definitions:
> // Generalized atomic volatile accesses valid in OrderAccess
> // All other types can be expressed in terms of these.
> inline void OrderAccess::store(volatile jbyte*   p, jbyte   v) { *p = v; }
> inline void OrderAccess::store(volatile jshort*  p, jshort  v) { *p = v; }
> inline void OrderAccess::store(volatile jint*    p, jint    v) { *p = v; }
> inline void OrderAccess::store(volatile jlong*   p, jlong   v) {
> Atomic::store(v, p); }
> inline void OrderAccess::store(volatile jdouble* p, jdouble v) {
> Atomic::store(jlong_cast(v), (volatile jlong*)p); }
> inline void OrderAccess::store(volatile jfloat*  p, jfloat  v) { *p = v; }
> ...and these are the exact types you can specialize.
> If you do not think this is clear enough or would like to move the
> comment somewhere else I'm fine with that. What do you think?
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.inline.hpp
>>    138 // The following methods can be specialized using simple template specialization
>>    139 // in the platform specific files for optimization purposes. Otherwise the
>>    140 // generalized variant is used.
>>    141 template<typename T> inline T    OrderAccess::specialized_load_acquire       (volatile T* p)       { return ordered_load<T, X_ACQUIRE>(p);    }
>>    142 template<typename T> inline void OrderAccess::specialized_release_store      (volatile T* p, T v)  { ordered_store<T, RELEASE_X>(p, v);       }
>>    143 template<typename T> inline void OrderAccess::specialized_release_store_fence(volatile T* p, T v)  { ordered_store<T, RELEASE_X_FENCE>(p, v); }
>> I *think* it doesn't matter in practice, but I would put these
>> definitions before the potential instantiations.  The relevant
>> declarations are obviously in scope where those instantiations occur,
>> and everything is declared inline, but I worry that some
>> insufficiently clever compiler might end up generating out of line
>> code in some cases because the definition was after use.
> As your initial intuition said - it doesn't matter in practice: inline
> function definitions are not instantiations. Instantiation will happen
> when the user actually calls e.g. OrderAccess::load_acquire, at which
> point all template definitions/inline definitions are known and used for
> instantiating them. Why? Because inlined functions are instantiated when
> called, not when defined.
> So it won't be a problem. I'm pretty sure any compiler not doing this
> wouldn't work. ;)
>> ------------------------------------------------------------------------------
>> src/share/vm/runtime/orderAccess.inline.hpp
>>    101 inline jubyte   OrderAccess::load_acquire(volatile jubyte*  p) { return (jubyte) specialized_load_acquire((volatile jbyte*)p);  }
>>    102 inline jushort  OrderAccess::load_acquire(volatile jushort* p) { return (jushort)specialized_load_acquire((volatile jshort*)p); }
>>    103 inline juint    OrderAccess::load_acquire(volatile juint*   p) { return (juint)  specialized_load_acquire((volatile jint*)p);   }
>>    104 inline julong   OrderAccess::load_acquire(volatile julong*  p) { return (julong) specialized_load_acquire((volatile jlong*)p);  }
>> I would probably define these as calling load_acquire with casts,
>> rather than directly calling specialized_load_acquire.  Similarly for
>> store_release and load_ptr_acquire variants.  This would make it
>> easier to fiddle with the how the specializations are implemented, and
>> shorten the line lengths by a dozen characters.
> I agree it would undoubtably be 12 characters smaller, but consistency
> seems more important to me. I think it's nice they all call the same
> function with different types instead of introducing exceptions to the
> rule in order to save 12 characters of line width for some specific
> overloads. Would you agree?
>> I would probably also have used some simple macrology to avoid near
>> duplication of code.
> Again, not convinced macros would improve readability/understandability
> here as argued before. ;)
>> ------------------------------------------------------------------------------
>> src/os_cpu/linux_ppc/vm/orderAccess_linux_ppc.inline.hpp
>> I see no uses of the inlasm_eieio macro.
>> I see no uses of the inlasm_isync macro.
> No I thought they were there as available tools for the future if they
> will then be wanted as well as to comprehensively list the available
> fences. I don't think eieio was ever used. If you want me to though, I
> will remove them.
> Many thanks for reviewing these changes Kim! :)
> Cheers,
> /Erik

More information about the hotspot-dev mailing list