From 4c0eb46c1924ca71bb4092b0c91fceb5b790e3f8 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 16 Nov 2018 10:20:45 +0100 Subject: [PATCH] Index nonnull/getter methods by String Looking up by String offers slight advantage over Method lookup, because we'll end up with a constant hashCode() over objects which are guaranteed to be interned. This rework implies we no longer get to have a map whose keySet reflects property methods -- which is fine, as the keyset would take up more memory than a plain array, so we retain the methods in an array -- hence this ends up being a net win in both cases. Furthermore it flushes out the fact that nonnull methods should not really cache their result -- for example if the user performs a nonnull() access and then runs hashCode(), we'll end up performing two decodes, as nonnull() cache will not be picked up by hashCode(). We therefore construct nonnull->getter String map, which we consult on nonnull accesses and only wrap it without caching. This has the benefit of keeping the cache smaller while maintaining its effectiveness. JIRA: MDSAL-398 Change-Id: I36526ce15735d4ef8e4ba847bcc24ea69b950e94 Signed-off-by: Robert Varga --- .../codec/impl/DataObjectCodecContext.java | 74 +++++++++++-------- .../codec/impl/KeyedListNodeCodecContext.java | 14 ++-- .../dom/codec/impl/LazyDataObject.java | 61 ++++++++------- 3 files changed, 85 insertions(+), 64 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 43ea4752bd..b4aed692f9 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 @@ -13,17 +13,15 @@ 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 java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.lang.reflect.Proxy; -import java.util.Collection; +import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; @@ -31,15 +29,13 @@ 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.atomic.AtomicReferenceFieldUpdater; -import java.util.function.Function; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; 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.naming.BindingMapping; import org.opendaylight.mdsal.binding.spec.reflect.BindingReflections; import org.opendaylight.yangtools.concepts.Immutable; import org.opendaylight.yangtools.util.ClassLoaderUtils; @@ -84,15 +80,17 @@ abstract class DataObjectCodecContext METHOD_BY_ALPHABET = Comparator.comparing(Method::getName); private static final Augmentations EMPTY_AUGMENTATIONS = new Augmentations(ImmutableMap.of(), ImmutableMap.of()); + private static final Method[] EMPTY_METHODS = new Method[0]; private final ImmutableMap> leafChild; private final ImmutableMap byYang; - private final ImmutableSortedMap byMethod; - private final ImmutableMap nonnullMethods; + private final ImmutableMap byMethod; + private final ImmutableMap nonnullToGetter; private final ImmutableMap, DataContainerCodecPrototype> byStreamClass; private final ImmutableMap, DataContainerCodecPrototype> byBindingArgClass; private final ImmutableMap possibleAugmentations; private final MethodHandle proxyConstructor; + private final Method[] propertyMethods; @SuppressWarnings("rawtypes") private static final AtomicReferenceFieldUpdater @@ -111,13 +109,13 @@ abstract class DataObjectCodecContext, Method> clsToMethod = BindingReflections.getChildrenClassToMethod(bindingClass); final Map byYangBuilder = new HashMap<>(); - final SortedMap byMethodBuilder = new TreeMap<>(METHOD_BY_ALPHABET); + final Map tmpMethodToSupplier = new HashMap<>(); final Map, DataContainerCodecPrototype> byStreamClassBuilder = new HashMap<>(); final Map, DataContainerCodecPrototype> byBindingArgClassBuilder = new HashMap<>(); // Adds leaves to mapping for (final LeafNodeCodecContext leaf : leafChild.values()) { - byMethodBuilder.put(leaf.getGetter(), leaf); + tmpMethodToSupplier.put(leaf.getGetter(), leaf); byYangBuilder.put(leaf.getDomPathArgument(), leaf); } @@ -125,7 +123,7 @@ abstract class DataObjectCodecContext childProto = loadChildPrototype(childDataObj.getKey()); - byMethodBuilder.put(method, childProto); + tmpMethodToSupplier.put(method, childProto); byStreamClassBuilder.put(childProto.getBindingClass(), childProto); byYangBuilder.put(childProto.getYangArg(), childProto); if (childProto.isChoice()) { @@ -135,29 +133,44 @@ abstract class DataObjectCodecContext byMethodBuilder = ImmutableMap.builderWithExpectedSize(methodCount); + this.propertyMethods = methodCount == 0 ? EMPTY_METHODS : new Method[methodCount]; + + int offset = 0; + for (Entry entry : tmpMethodToSupplier.entrySet()) { + final Method method = entry.getKey(); + propertyMethods[offset++] = method; + byMethodBuilder.put(method.getName(), entry.getValue()); + } + + // Make sure properties are alpha-sorted + Arrays.sort(propertyMethods, METHOD_BY_ALPHABET); + + this.byMethod = byMethodBuilder.build(); this.byYang = ImmutableMap.copyOf(byYangBuilder); this.byStreamClass = ImmutableMap.copyOf(byStreamClassBuilder); byBindingArgClassBuilder.putAll(byStreamClass); this.byBindingArgClass = ImmutableMap.copyOf(byBindingArgClassBuilder); final Map, Method> clsToNonnull = BindingReflections.getChildrenClassToNonnullMethod(bindingClass); - final Map nonnullMethodsBuilder = new HashMap<>(); + final Map nonnullToGetterBuilder = new HashMap<>(); for (final Entry, Method> entry : clsToNonnull.entrySet()) { final Method method = entry.getValue(); if (!method.isDefault()) { LOG.warn("Ignoring non-default method {} in {}", method, bindingClass); continue; } - final DataContainerCodecPrototype supplier = byStreamClass.get(entry.getKey()); - if (supplier != null) { - nonnullMethodsBuilder.put(method, supplier); - } else { - LOG.warn("Failed to look up data handler for method {}", method); - } - } - nonnullMethods = ImmutableMap.copyOf(nonnullMethodsBuilder); + // Derive getter name from the nonnull method and verify we have the corresponding getter. Note that + // the intern() call is important, as it makes sure we use the same instance to bridge to byMethod map. + final String methodName = method.getName(); + final String getterName = BindingMapping.getGetterMethodForNonnull(methodName).intern(); + verify(byMethod.containsKey(getterName), "Cannot find getter %s for %s", getterName, methodName); + nonnullToGetterBuilder.put(methodName, getterName); + } + nonnullToGetter = ImmutableMap.copyOf(nonnullToGetterBuilder); if (Augmentable.class.isAssignableFrom(bindingClass)) { this.possibleAugmentations = factory().getRuntimeContext().getAvailableAugmentationTypes(getSchema()); @@ -500,15 +513,15 @@ abstract class DataObjectCodecContext domData) { - return method.isDefault() ? getBindingChildValue(nonnullMethods, method, domData, dummy -> ImmutableList.of()) - : getBindingChildValue(byMethod, method, domData, NodeCodecContext::defaultObject); + // Unlike BindingMapping.getGetterMethodForNonnull() this returns an interned String + @NonNull String getterNameForNonnullName(final String nonnullMethod) { + return verifyNotNull(nonnullToGetter.get(nonnullMethod), "Failed to look up getter method for %s", + nonnullMethod); } @SuppressWarnings("rawtypes") - private static Object getBindingChildValue(final ImmutableMap map, final Method method, - final NormalizedNodeContainer domData, final Function, Object> getDefaultObject) { - final NodeCodecContext childContext = verifyNotNull(map.get(method), + @Nullable Object getBindingChildValue(final String method, final NormalizedNodeContainer domData) { + final NodeCodecContext childContext = verifyNotNull(byMethod.get(method), "Cannot find data handler for method %s", method).get(); @SuppressWarnings("unchecked") @@ -516,8 +529,7 @@ abstract class DataObjectCodecContext getHashCodeAndEqualsMethods() { - return byMethod.keySet(); + final Method[] propertyMethods() { + return propertyMethods; } @Override diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/KeyedListNodeCodecContext.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/KeyedListNodeCodecContext.java index f9d5a9ae48..7ac98b040d 100644 --- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/KeyedListNodeCodecContext.java +++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/KeyedListNodeCodecContext.java @@ -7,9 +7,9 @@ */ package org.opendaylight.mdsal.binding.dom.codec.impl; -import java.lang.reflect.Method; +import static org.opendaylight.mdsal.binding.spec.naming.BindingMapping.IDENTIFIABLE_KEY_NAME; + import java.util.List; -import org.opendaylight.mdsal.binding.spec.naming.BindingMapping; import org.opendaylight.yangtools.concepts.Codec; import org.opendaylight.yangtools.yang.binding.DataObject; import org.opendaylight.yangtools.yang.binding.Identifiable; @@ -24,7 +24,6 @@ import org.opendaylight.yangtools.yang.model.api.ListSchemaNode; final class KeyedListNodeCodecContext> extends ListNodeCodecContext { private final Codec> codec; - private final Method keyGetter; KeyedListNodeCodecContext(final DataContainerCodecPrototype prototype) { super(prototype); @@ -32,7 +31,8 @@ final class KeyedListNodeCodecContext> ex final Class bindingClass = getBindingClass(); this.codec = factory().getPathArgumentCodec(bindingClass, getSchema()); try { - this.keyGetter = bindingClass.getMethod(BindingMapping.IDENTIFIABLE_KEY_NAME); + // This just verifies the method is present + bindingClass.getMethod(IDENTIFIABLE_KEY_NAME); } catch (NoSuchMethodException e) { throw new IllegalStateException("Required method not available", e); } @@ -60,12 +60,12 @@ final class KeyedListNodeCodecContext> ex @Override @SuppressWarnings("rawtypes") - Object getBindingChildValue(final Method method, final NormalizedNodeContainer dom) { - if (dom instanceof MapEntryNode && keyGetter.equals(method)) { + Object getBindingChildValue(final String methodName, final NormalizedNodeContainer dom) { + if (dom instanceof MapEntryNode && IDENTIFIABLE_KEY_NAME.equals(methodName)) { NodeIdentifierWithPredicates identifier = ((MapEntryNode) dom).getIdentifier(); return codec.deserialize(identifier).getKey(); } - return super.getBindingChildValue(method, dom); + return super.getBindingChildValue(methodName, dom); } @Override diff --git a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/LazyDataObject.java b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/LazyDataObject.java index e191054a7e..9467c868e0 100644 --- a/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/LazyDataObject.java +++ b/binding/mdsal-binding-dom-codec/src/main/java/org/opendaylight/mdsal/binding/dom/codec/impl/LazyDataObject.java @@ -14,6 +14,7 @@ import static org.opendaylight.mdsal.binding.spec.naming.BindingMapping.DATA_CON import com.google.common.base.MoreObjects; import com.google.common.base.MoreObjects.ToStringHelper; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import java.lang.reflect.InvocationHandler; import java.lang.reflect.InvocationTargetException; @@ -25,6 +26,7 @@ import java.util.Objects; import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; +import org.eclipse.jdt.annotation.NonNull; import org.opendaylight.mdsal.binding.dom.codec.util.AugmentationReader; import org.opendaylight.mdsal.binding.spec.reflect.BindingReflections; import org.opendaylight.yangtools.yang.binding.Augmentable; @@ -43,8 +45,10 @@ class LazyDataObject implements InvocationHandler, Augment private static final String EQUALS = "equals"; private static final String HASHCODE = "hashCode"; private static final String AUGMENTATIONS = "augmentations"; - private static final Object NULL_VALUE = new Object(); + private static final @NonNull Object NULL_VALUE = new Object(); + // Method.getName() is guaranteed to be interned and all getter methods have zero arguments, name is sufficient to + // identify the data, skipping Method.hashCode() computation. private final ConcurrentHashMap cachedData = new ConcurrentHashMap<>(); private final NormalizedNodeContainer> data; private final DataObjectCodecContext context; @@ -65,7 +69,8 @@ class LazyDataObject implements InvocationHandler, Augment public Object invoke(final Object proxy, final Method method, final Object[] args) { switch (method.getParameterCount()) { case 0: - switch (method.getName()) { + final String methodName = method.getName(); + switch (methodName) { case DATA_CONTAINER_GET_IMPLEMENTED_INTERFACE_NAME: return context.getBindingClass(); case TO_STRING: @@ -75,7 +80,7 @@ class LazyDataObject implements InvocationHandler, Augment case AUGMENTATIONS: return getAugmentationsImpl(); default: - return getBindingData(method); + return method.isDefault() ? nonnullBindingData(methodName) : getBindingData(methodName); } case 1: switch (method.getName()) { @@ -103,13 +108,13 @@ class LazyDataObject implements InvocationHandler, Augment return false; } try { - for (final Method m : context.getHashCodeAndEqualsMethods()) { - final Object thisValue = getBindingData(m); + for (final Method m : context.propertyMethods()) { + final Object thisValue = getBindingData(m.getName()); final Object otherValue = m.invoke(other); /* - * added for valid byte array comparison, when list key type is binary - * deepEquals is not used since it does excessive amount of instanceof calls. - */ + * added for valid byte array comparison, when list key type is binary + * deepEquals is not used since it does excessive amount of instanceof calls. + */ if (thisValue instanceof byte[] && otherValue instanceof byte[]) { if (!Arrays.equals((byte[]) thisValue, (byte[]) otherValue)) { return false; @@ -149,8 +154,8 @@ class LazyDataObject implements InvocationHandler, Augment final int prime = 31; int result = 1; - for (final Method m : context.getHashCodeAndEqualsMethods()) { - final Object value = getBindingData(m); + for (final Method m : context.propertyMethods()) { + final Object value = getBindingData(m.getName()); result = prime * result + Objects.hashCode(value); } if (Augmentable.class.isAssignableFrom(context.getBindingClass())) { @@ -161,23 +166,26 @@ class LazyDataObject implements InvocationHandler, Augment return ret; } - private Object getBindingData(final Method method) { - // Guaranteed to be interned and since method has zero arguments, name is sufficient to identify the data, - // skipping Method.hashCode() computation. - final String methodName = method.getName(); - Object cached = cachedData.get(methodName); - if (cached == null) { - final Object readedValue = context.getBindingChildValue(method, data); - cached = readedValue == null ? NULL_VALUE : readedValue; + private Object nonnullBindingData(final String methodName) { + final Object value = getBindingData(context.getterNameForNonnullName(methodName)); + return value != null ? value : ImmutableList.of(); + } - final Object raced = cachedData.putIfAbsent(methodName, cached); - if (raced != null) { - // Load/store raced, we should return the stored value - cached = raced; - } + // Internal invocation, can only target getFoo() methods + private Object getBindingData(final String methodName) { + final Object cached = cachedData.get(methodName); + if (cached != null) { + return unmaskNull(cached); } - return cached == NULL_VALUE ? null : cached; + final Object value = context.getBindingChildValue(methodName, data); + final Object raced = cachedData.putIfAbsent(methodName, value == null ? NULL_VALUE : value); + // If we raced we need to return previously-stored value + return raced != null ? unmaskNull(raced) : value; + } + + private static Object unmaskNull(final @NonNull Object masked) { + return masked == NULL_VALUE ? null : masked; } private Map>, Augmentation> getAugmentationsImpl() { @@ -229,8 +237,9 @@ class LazyDataObject implements InvocationHandler, Augment final Class bindingClass = context.getBindingClass(); final ToStringHelper helper = MoreObjects.toStringHelper(bindingClass).omitNullValues(); - for (final Method m : context.getHashCodeAndEqualsMethods()) { - helper.add(m.getName(), getBindingData(m)); + for (final Method m : context.propertyMethods()) { + final String methodName = m.getName(); + helper.add(methodName, getBindingData(methodName)); } if (Augmentable.class.isAssignableFrom(bindingClass)) { helper.add("augmentations", getAugmentationsImpl()); -- 2.36.6