From d349008b4936f7e128ec292a66e05f3e9bd57e73 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Wed, 14 Nov 2018 16:40:29 +0100 Subject: [PATCH] Use ImmutableMaps for lazy augmentations initialization Rather than tracking lookups in two separate ConcurrentMaps, we perform locked atomic updates in copy-on-write immutable structure. The two lookup maps are kept in ImmutableMaps, which offer tighter packing and do not incur any further volatile reads beyond getting a reference to them. This results in lower memory overhead, as the maps are more tightly packed and also biases performance towards the dominant use case, which is pure lookups. JIRA: MDSAL-391 Change-Id: I65ecb25a5d364040be44df144000d1bbbc2a922e Signed-off-by: Robert Varga --- .../codec/impl/DataObjectCodecContext.java | 116 +++++++++++++++--- 1 file changed, 102 insertions(+), 14 deletions(-) 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 774573a9f7..43ea4752bd 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 @@ -10,13 +10,13 @@ package org.opendaylight.mdsal.binding.dom.codec.impl; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Verify.verify; import static com.google.common.base.Verify.verifyNotNull; +import static java.util.Objects.requireNonNull; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableMap.Builder; import com.google.common.collect.ImmutableSortedMap; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; @@ -26,14 +26,14 @@ import java.lang.reflect.Proxy; import java.util.Collection; import java.util.Comparator; import java.util.HashMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; import java.util.SortedMap; import java.util.TreeMap; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.function.Function; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; @@ -41,6 +41,7 @@ import org.opendaylight.mdsal.binding.generator.api.ClassLoadingStrategy; import org.opendaylight.mdsal.binding.model.api.JavaTypeName; import org.opendaylight.mdsal.binding.model.api.Type; import org.opendaylight.mdsal.binding.spec.reflect.BindingReflections; +import org.opendaylight.yangtools.concepts.Immutable; import org.opendaylight.yangtools.util.ClassLoaderUtils; import org.opendaylight.yangtools.yang.binding.Augmentable; import org.opendaylight.yangtools.yang.binding.Augmentation; @@ -67,10 +68,22 @@ import org.slf4j.LoggerFactory; abstract class DataObjectCodecContext extends DataContainerCodecContext { + private static final class Augmentations implements Immutable { + final ImmutableMap> byYang; + final ImmutableMap, DataContainerCodecPrototype> byStream; + + Augmentations(final ImmutableMap> byYang, + final ImmutableMap, DataContainerCodecPrototype> byStream) { + this.byYang = requireNonNull(byYang); + this.byStream = requireNonNull(byStream); + } + } + private static final Logger LOG = LoggerFactory.getLogger(DataObjectCodecContext.class); private static final MethodType CONSTRUCTOR_TYPE = MethodType.methodType(void.class, InvocationHandler.class); private static final MethodType DATAOBJECT_TYPE = MethodType.methodType(DataObject.class, InvocationHandler.class); private static final Comparator METHOD_BY_ALPHABET = Comparator.comparing(Method::getName); + private static final Augmentations EMPTY_AUGMENTATIONS = new Augmentations(ImmutableMap.of(), ImmutableMap.of()); private final ImmutableMap> leafChild; private final ImmutableMap byYang; @@ -81,9 +94,11 @@ abstract class DataObjectCodecContext possibleAugmentations; private final MethodHandle proxyConstructor; - private final ConcurrentMap> byYangAugmented = - new ConcurrentHashMap<>(); - private final ConcurrentMap, DataContainerCodecPrototype> byStreamAugmented = new ConcurrentHashMap<>(); + @SuppressWarnings("rawtypes") + private static final AtomicReferenceFieldUpdater + AUGMENTATIONS_UPDATER = AtomicReferenceFieldUpdater.newUpdater(DataObjectCodecContext.class, + Augmentations.class, "augmentations"); + private volatile Augmentations augmentations = EMPTY_AUGMENTATIONS; private volatile ImmutableMap, DataContainerCodecPrototype> mismatchedAugmented = ImmutableMap.of(); @@ -161,13 +176,80 @@ abstract class DataObjectCodecContext> addByYang = new HashMap<>(); + final Map, DataContainerCodecPrototype> addByStream = new HashMap<>(); + + // Iterate over all possibilities, checking for modifications. for (final Type augment : possibleAugmentations.values()) { final DataContainerCodecPrototype augProto = getAugmentationPrototype(augment); if (augProto != null) { - byYangAugmented.putIfAbsent(augProto.getYangArg(), augProto); - byStreamAugmented.putIfAbsent(augProto.getBindingClass(), augProto); + final PathArgument yangArg = augProto.getYangArg(); + final Class bindingClass = augProto.getBindingClass(); + if (!oldAugmentations.byYang.containsKey(yangArg)) { + if (addByYang.putIfAbsent(yangArg, augProto) == null) { + LOG.trace("Discovered new YANG mapping {} -> {} in {}", yangArg, augProto, this); + } + } + if (!oldAugmentations.byStream.containsKey(bindingClass)) { + if (addByStream.putIfAbsent(bindingClass, augProto) == null) { + LOG.trace("Discovered new class mapping {} -> {} in {}", bindingClass, augProto, this); + } + } + } + } + + while (true) { + if (addByYang.isEmpty() && addByStream.isEmpty()) { + LOG.trace("No new augmentations discovered in {}", this); + return; + } + + // We have some additions, propagate them out + final Augmentations newAugmentations = new Augmentations(concatMaps(oldAugmentations.byYang, addByYang), + concatMaps(oldAugmentations.byStream, addByStream)); + if (AUGMENTATIONS_UPDATER.compareAndSet(this, oldAugmentations, newAugmentations)) { + // Success, we are done + return; + } + + // We have raced installing new augmentations, read them again, remove everything present in the installed + // once and try again. This may mean that we end up not doing anything, but that's fine. + oldAugmentations = augmentations; + + // We could use Map.removeAll(oldAugmentations.byYang.keySet()), but that forces the augmentation's keyset + // to be materialized, which we otherwise do not need. Hence we do this the other way around, instantiating + // our temporary maps' keySets and iterating over them. That's fine as we'll be throwing those maps away. + removeMapKeys(addByYang, oldAugmentations.byYang); + removeMapKeys(addByStream, oldAugmentations.byStream); + } + } + + private static ImmutableMap concatMaps(final ImmutableMap old, final Map add) { + if (add.isEmpty()) { + return old; + } + + final Builder builder = ImmutableMap.builderWithExpectedSize(old.size() + add.size()); + builder.putAll(old); + builder.putAll(add); + return builder.build(); + } + + private static void removeMapKeys(final Map removeFrom, final ImmutableMap map) { + final Iterator it = removeFrom.keySet().iterator(); + while (it.hasNext()) { + if (map.containsKey(it.next())) { + it.remove(); } } } @@ -301,13 +383,15 @@ abstract class DataObjectCodecContext yangAugmentationChild(final AugmentationIdentifier arg) { - final DataContainerCodecPrototype firstTry = byYangAugmented.get(arg); + final DataContainerCodecPrototype firstTry = augmentations.byYang.get(arg); if (firstTry != null) { return firstTry; } if (possibleAugmentations.containsKey(arg)) { + // Try to load augmentations, which will potentially update knownAugmentations, hence we re-load that field + // again. reloadAllAugmentations(); - return byYangAugmented.get(arg); + return augmentations.byYang.get(arg); } return null; } @@ -349,7 +433,9 @@ abstract class DataObjectCodecContext augmentationByClassOrEquivalentClass( final @NonNull Class childClass) { - final DataContainerCodecPrototype childProto = byStreamAugmented.get(childClass); + // Perform a single load, so we can reuse it if we end up going to the reflection-based slow path + final ImmutableMap, DataContainerCodecPrototype> local = augmentations.byStream; + final DataContainerCodecPrototype childProto = local.get(childClass); if (childProto != null) { return childProto; } @@ -367,13 +453,14 @@ abstract class DataObjectCodecContext augTarget = BindingReflections.findAugmentationTarget((Class) childClass); if (getBindingClass().equals(augTarget)) { - for (final DataContainerCodecPrototype realChild : byStreamAugmented.values()) { + for (final DataContainerCodecPrototype realChild : local.values()) { if (Augmentation.class.isAssignableFrom(realChild.getBindingClass()) && BindingReflections.isSubstitutionFor(childClass, realChild.getBindingClass())) { return cacheMismatched(childClass, realChild); } } } + LOG.trace("Failed to resolve {} as a valid augmentation in {}", childClass, this); return null; } @@ -392,6 +479,7 @@ abstract class DataObjectCodecContext {} in {}", childClass, prototype, this); return prototype; } @@ -459,7 +547,7 @@ abstract class DataObjectCodecContext value : byStreamAugmented.values()) { + for (final DataContainerCodecPrototype value : augmentations.byStream.values()) { final Optional> augData = data.getChild(value.getYangArg()); if (augData.isPresent()) { map.put(value.getBindingClass(), value.get().deserializeObject(augData.get())); -- 2.36.6