RFR (M): 8029396: PPC64 (part 212): Several memory ordering fixes in C-code.
david.holmes at oracle.com
Thu Dec 19 02:52:03 PST 2013
Somewhat late but I was away for two weeks.
Is the use of the new release parameter always set to true from shared
code? If so I don't have a problem with it being used to optimize the
stores under certain conditions. But if pcc64 will pass true where other
archs won't then again I object to this because it should be an
algorithmic correctness requirement that is always present.
General: I find a lot of the commentary excessively platform specific.
Eg We don't expect any C++ compiler we currently use to emit memory
barriers for C++ volatiles. If they start doing that we will have a
bazillion unnecessary injected barriers in our code!
It is not clear to me that the BiasedLocking change is needed. AFAICS
there is only one path where revoke_bias is not called at a safepoint
and the comments around that seem to indicate that it was considered
safe to do so. It may be they assumed TSO when making that decision but
I'd be interested to know how this change was arrived at.
The thread state changes have me most concerned as they are heavily used
and so the performance impact here could be extensive. Many of them
occur on paths that involve membars or membar-equivalent actions so they
would seem redundant then. Again I would like to see some analysis
showing these are in fact essential for correctness. There may well be
some situations where they are, but to me this seems an even better
candidate for adding the "release" parameter when needed!
On 3/12/2013 2:51 AM, Lindenmaier, Goetz wrote:
> This change contains a row of fixes to the memory ordering in runtime, GC etc.
> These are:
> - Accessing arrays in CMS (compactibleFreeListSpace.cpp)
> - CMS: Do release when marking a card dirty. The release must only be done if GC is running. (several files)
> - Method counter initialization (method.hpp).
> - Order accessing f1/f2 in constant pool cache.
> - Release stores in OopMapCache constructor (instanceKLass.cpp).
> - BiasedLocking: Release setting object header to displaced mark.
> - Release state of nmethod sweeper (sweeper.cpp).
> - Do barriers when writing the thread state (thread.hpp).
> Please review and test this change.
> If requested, I can part this into smaller changes. But for now
> I wanted to put them all into one change as they all address the
> problems with the PPC memory model.
> Best regards,
More information about the hotspot-dev