From 31fcf6368b746ab23cc687293f4b4e20e2cf793a Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 26 Apr 2019 21:36:30 +0200 Subject: [PATCH] Drop a FIXME in DataContainerCodecPrototype Acquisition of the backer is in the hot path now, hence we should consider if we can safely remove or relax the volatile read here. Change-Id: I9efe3cd2f45ab97f6c4c9bcc8f18e4cf62542b60 Signed-off-by: Robert Varga --- .../dom/codec/impl/CodecDataObject.java | 3 +- .../impl/DataContainerCodecPrototype.java | 31 +++++++++++++++++++ .../codec/impl/DataObjectCodecContext.java | 2 ++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/CodecDataObject.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/CodecDataObject.java index a9446a1394..b63e2928a7 100644 --- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/CodecDataObject.java +++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/CodecDataObject.java @@ -75,7 +75,8 @@ public abstract class CodecDataObject implements DataObjec .toString(); } - // TODO: consider switching to VarHandles for Java 9+ + // TODO: consider switching to VarHandles for Java 9+, as that would disconnect us from the need to use a volatile + // field and use acquire/release mechanics -- see http://gee.cs.oswego.edu/dl/html/j9mm.html for details. protected final Object codecMember(final AtomicReferenceFieldUpdater, Object> updater, final NodeContextSupplier supplier) { final Object cached = updater.get(this); diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java index 35b37a1e76..73b3c577b9 100644 --- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java +++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataContainerCodecPrototype.java @@ -46,6 +46,37 @@ final class DataContainerCodecPrototype implements NodeCon private final PathArgument yangArg; private final ChildAddressabilitySummary childAddressabilitySummary; + /* + * FIXME: This field is now utterly in the hot path of CodecDataObject's instantiation of data. It is volatile + * support double-checked locking, as detailed in + * https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java. + * + * Removing volatile here would seem to be worthwhile, as it would mean that in the initialized case we end + * up with no happens-before synchronization between threads, allowing JIT to perform better. + * + * Since we are on Java 5+, DataContainerCodecContext and its subclasses play a role here. All of their + * fields are either final or volatile, so intuition would indicate they themselves could (already?) be + * acting in the same capacity as "FinalWrapper" in the above example does -- in which case we can safely + * remove the volatile access altogether, after documenting this fact there (and potentially placing some + * guards). + * + * The presence of volatile fields seems to violate this, as + * http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html mentions "such that all of the + * fields of Helper are final". + * + * There are only two violations, both relating to dynamic discovery of augmentations, which should be + * eliminated once we have constant class loader visibility (and thus can discover all valid augmentations + * and aliases). Alternatively, adding an indirection could be considered after the effects are fully + * understood. + * + * Alternatively we can consider the use of VarHandle acquire/release mechanics to remove this instance + * from global synchronization order -- that would be a Java 9+ concern, though. + * + * Finally, the benefits of addressing this are masked by CodecDataObject operating on volatiles, hence + * already doing a barrier very close to us -- thus reducing the scope in which code can be reordered. + * If CodecDataObject moves to VarHandles and uses acquire/release there, we will be put on the spot here, + * though. + */ private volatile DataContainerCodecContext instance; @SuppressWarnings("unchecked") diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataObjectCodecContext.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataObjectCodecContext.java index c5360d940f..cb9fa451fb 100644 --- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataObjectCodecContext.java +++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/DataObjectCodecContext.java @@ -98,6 +98,8 @@ abstract class DataObjectCodecContext possibleAugmentations; private final MethodHandle proxyConstructor; + // FIXME: the presence of these two volatile fields may be preventing us from being able to improve + // DataContainerCodecPrototype.get() publication. @SuppressWarnings("rawtypes") private static final AtomicReferenceFieldUpdater AUGMENTATIONS_UPDATER = AtomicReferenceFieldUpdater.newUpdater(DataObjectCodecContext.class, -- 2.36.6