From df9f8c417e2a5d968b7c80da88753a1468f8f280 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Sat, 24 Jan 2015 13:22:14 +0100 Subject: [PATCH] DATACMNS-637 - Performance improvements. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Made ObjectInstantiator interface public as otherwise the generated class to implement it cannot access it and thus the usage of the ByteCodeGeneratingEntityInstantiator fails completely. PreferredConstructor.isEnclosingClassParameter(…) now eagerly returns if the parameter itself is not an enclosing one and thus avoids a collection lookup and equals check. Moved equals check for the type to the very end of the equals check to increase the chances that other inequality guards kick in earlier. AbstractMappingContext now leaves the non-null-check for getPersistentEntity(…) to the factory method of ClassTypeInformation. We now pre-calculate the hash codes for TypeInformation implementations as far as possible as the instances are used as cache keys quite a lot. The same applies to AbstractPersistentProperty. BasicPersistentEntity now uses an ArrayList we sort during the verify() phase to mimic the previous behavior wich was implemented using a TreeSet as ArrayLists are way more performant when iterating over all elements which doWithProperties(…) is doing which is used quite a lot. BeanWrapper now avoids the getter lookup if field access is used. SimpleTypeHolder now uses a CopyOnWriteArrySet to leniently add types detected to be simple to the set of simple types to avoid ongoing checks against the inheritance hierarchy. --- .../BytecodeGeneratingEntityInstantiator.java | 4 +- .../MappingContextTypeInformationMapper.java | 48 +++++--- .../data/mapping/PreferredConstructor.java | 12 +- .../context/AbstractMappingContext.java | 3 +- .../model/AbstractPersistentProperty.java | 6 +- .../mapping/model/BasicPersistentEntity.java | 28 ++++- .../data/mapping/model/BeanWrapper.java | 9 +- .../data/mapping/model/SimpleTypeHolder.java | 10 +- .../springframework/data/util/CacheValue.java | 107 ++++++++++++++++++ .../util/ParentTypeAwareTypeInformation.java | 12 +- .../data/util/TypeDiscoverer.java | 19 +--- .../model/BasicPersistentEntityUnitTests.java | 12 +- 12 files changed, 205 insertions(+), 65 deletions(-) create mode 100644 src/main/java/org/springframework/data/util/CacheValue.java diff --git a/src/main/java/org/springframework/data/convert/BytecodeGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/BytecodeGeneratingEntityInstantiator.java index 657bb6d13..cdec6d8c5 100644 --- a/src/main/java/org/springframework/data/convert/BytecodeGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/BytecodeGeneratingEntityInstantiator.java @@ -1,5 +1,5 @@ /* - * Copyright 2014 the original author or authors. + * Copyright 2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -223,7 +223,7 @@ public enum BytecodeGeneratingEntityInstantiator implements EntityInstantiator { /** * @author Thomas Darimont */ - interface ObjectInstantiator { + public interface ObjectInstantiator { Object newInstance(Object... args); } diff --git a/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java b/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java index 4bfc15287..b8965e739 100644 --- a/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java +++ b/src/main/java/org/springframework/data/convert/MappingContextTypeInformationMapper.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 the original author or authors. + * Copyright 2011-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,12 +15,13 @@ */ package org.springframework.data.convert; -import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.concurrent.ConcurrentHashMap; import org.springframework.data.mapping.PersistentEntity; import org.springframework.data.mapping.context.MappingContext; +import org.springframework.data.util.CacheValue; import org.springframework.data.util.ClassTypeInformation; import org.springframework.data.util.TypeInformation; import org.springframework.util.Assert; @@ -34,7 +35,7 @@ import org.springframework.util.Assert; */ public class MappingContextTypeInformationMapper implements TypeInformationMapper { - private final Map, Object> typeMap; + private final Map, CacheValue> typeMap; private final MappingContext, ?> mappingContext; /** @@ -47,7 +48,7 @@ public class MappingContextTypeInformationMapper implements TypeInformationMappe Assert.notNull(mappingContext); - this.typeMap = new HashMap, Object>(); + this.typeMap = new ConcurrentHashMap, CacheValue>(); this.mappingContext = mappingContext; for (PersistentEntity entity : mappingContext.getPersistentEntities()) { @@ -61,10 +62,10 @@ public class MappingContextTypeInformationMapper implements TypeInformationMappe */ public Object createAliasFor(TypeInformation type) { - Object key = typeMap.get(type); + CacheValue key = typeMap.get(type); if (key != null) { - return key; + return key.getValue(); } PersistentEntity entity = mappingContext.getPersistentEntity(type); @@ -87,26 +88,36 @@ public class MappingContextTypeInformationMapper implements TypeInformationMappe */ private void safelyAddToCache(ClassTypeInformation key, Object alias) { - if (alias == null) { + CacheValue aliasToBeCached = CacheValue.ofNullable(alias); + + if (alias == null && !typeMap.containsKey(key)) { + typeMap.put(key, aliasToBeCached); return; } - Object existingAlias = typeMap.get(key); + CacheValue alreadyCachedAlias = typeMap.get(key); // Reject second alias for same type - if (existingAlias != null && !alias.equals(existingAlias)) { + if (alreadyCachedAlias != null && alreadyCachedAlias.isPresent() && !alreadyCachedAlias.hasValue(alias)) { throw new IllegalArgumentException(String.format( - "Trying to register alias '%s', but found already registered alias '%s' for type %s!", alias, existingAlias, - key)); + "Trying to register alias '%s', but found already registered alias '%s' for type %s!", alias, + alreadyCachedAlias, key)); } // Reject second type for same alias - if (typeMap.containsValue(alias)) { + if (typeMap.containsValue(aliasToBeCached)) { - for (Entry, Object> entry : typeMap.entrySet()) { - if (entry.getValue().equals(alias) && !entry.getKey().equals(key)) { + for (Entry, CacheValue> entry : typeMap.entrySet()) { + + CacheValue value = entry.getValue(); + + if (!value.isPresent()) { + continue; + } + + if (value.hasValue(alias) && !entry.getKey().equals(key)) { throw new IllegalArgumentException(String.format( "Detected existing type mapping of %s to alias '%s' but attempted to bind the same alias to %s!", key, alias, entry.getKey())); @@ -114,7 +125,7 @@ public class MappingContextTypeInformationMapper implements TypeInformationMappe } } - typeMap.put(key, alias); + typeMap.put(key, aliasToBeCached); } /* @@ -127,8 +138,11 @@ public class MappingContextTypeInformationMapper implements TypeInformationMappe return null; } - for (Entry, Object> entry : typeMap.entrySet()) { - if (entry.getValue().equals(alias)) { + for (Entry, CacheValue> entry : typeMap.entrySet()) { + + CacheValue cachedAlias = entry.getValue(); + + if (cachedAlias.hasValue(alias)) { return entry.getKey(); } } diff --git a/src/main/java/org/springframework/data/mapping/PreferredConstructor.java b/src/main/java/org/springframework/data/mapping/PreferredConstructor.java index 8cfb6e8eb..9507eff32 100644 --- a/src/main/java/org/springframework/data/mapping/PreferredConstructor.java +++ b/src/main/java/org/springframework/data/mapping/PreferredConstructor.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 the original author or authors. + * Copyright 2011-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -168,11 +168,11 @@ public class PreferredConstructor> { Assert.notNull(parameter); - if (parameters.isEmpty()) { + if (parameters.isEmpty() || !parameter.isEnclosingClassParameter()) { return false; } - return parameters.get(0).equals(parameter) && parameter.isEnclosingClassParameter(); + return parameters.get(0).equals(parameter); } /** @@ -312,10 +312,10 @@ public class PreferredConstructor> { boolean nameEquals = this.name == null ? that.name == null : this.name.equals(that.name); boolean keyEquals = this.key == null ? that.key == null : this.key.equals(that.key); - boolean typeEquals = nullSafeEquals(this.type, that.type); boolean entityEquals = this.entity == null ? that.entity == null : this.entity.equals(that.entity); - return nameEquals && keyEquals && typeEquals && entityEquals; + // Explicitly delay equals check on type as it might be expensive + return nameEquals && keyEquals && entityEquals && this.type.equals(that.type); } /* @@ -329,8 +329,8 @@ public class PreferredConstructor> { result += 31 * nullSafeHashCode(this.name); result += 31 * nullSafeHashCode(this.key); - result += 31 * nullSafeHashCode(this.type); result += 31 * nullSafeHashCode(this.entity); + result += 31 * this.type.hashCode(); return result; } diff --git a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java index e1219c1e7..d64ff3200 100644 --- a/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java +++ b/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 by the original author(s). + * Copyright 2011-2015 by the original author(s). * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -137,7 +137,6 @@ public abstract class AbstractMappingContext type) { - Assert.notNull(type); return getPersistentEntity(ClassTypeInformation.from(type)); } diff --git a/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java b/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java index 1093c5172..41a85d375 100644 --- a/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java +++ b/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 the original author or authors. + * Copyright 2011-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -53,6 +53,7 @@ public abstract class AbstractPersistentProperty

protected final Association

association; protected final PersistentEntity owner; private final SimpleTypeHolder simpleTypeHolder; + private final int hashCode; public AbstractPersistentProperty(Field field, PropertyDescriptor propertyDescriptor, PersistentEntity owner, SimpleTypeHolder simpleTypeHolder) { @@ -68,6 +69,7 @@ public abstract class AbstractPersistentProperty

this.association = isAssociation() ? createAssociation() : null; this.owner = owner; this.simpleTypeHolder = simpleTypeHolder; + this.hashCode = this.field == null ? this.propertyDescriptor.hashCode() : this.field.hashCode(); } protected abstract Association

createAssociation(); @@ -340,7 +342,7 @@ public abstract class AbstractPersistentProperty

*/ @Override public int hashCode() { - return this.field == null ? this.propertyDescriptor.hashCode() : this.field.hashCode(); + return this.hashCode; } /* diff --git a/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java b/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java index 01c277965..e4ed226ef 100644 --- a/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java +++ b/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 by the original author(s). + * Copyright 2011-2015 by the original author(s). * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,9 +17,12 @@ package org.springframework.data.mapping.model; import java.io.Serializable; import java.lang.annotation.Annotation; +import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeSet; @@ -52,7 +55,8 @@ public class BasicPersistentEntity> implement private final PreferredConstructor constructor; private final TypeInformation information; - private final Set

properties; + private final List

properties; + private final Comparator

comparator; private final Set> associations; private final Map propertyCache; @@ -83,7 +87,8 @@ public class BasicPersistentEntity> implement Assert.notNull(information); this.information = information; - this.properties = comparator == null ? new HashSet

() : new TreeSet

(comparator); + this.properties = new ArrayList

(); + this.comparator = comparator; this.constructor = new PreferredConstructorDiscoverer(information, this).getConstructor(); this.associations = comparator == null ? new HashSet>() : new TreeSet>( new AssociationComparator

(comparator)); @@ -171,6 +176,11 @@ public class BasicPersistentEntity> implement public void addPersistentProperty(P property) { Assert.notNull(property); + + if (properties.contains(property)) { + return; + } + properties.add(property); if (!propertyCache.containsKey(property.getName())) { @@ -219,7 +229,10 @@ public class BasicPersistentEntity> implement * @see org.springframework.data.mapping.MutablePersistentEntity#addAssociation(org.springframework.data.mapping.model.Association) */ public void addAssociation(Association

association) { - associations.add(association); + + if (!associations.contains(association)) { + associations.add(association); + } } /* @@ -363,7 +376,12 @@ public class BasicPersistentEntity> implement * (non-Javadoc) * @see org.springframework.data.mapping.MutablePersistentEntity#verify() */ - public void verify() {} + public void verify() { + + if (comparator != null) { + Collections.sort(properties, comparator); + } + } /* * (non-Javadoc) diff --git a/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java b/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java index 709c222c5..ab902aeeb 100644 --- a/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java +++ b/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 by the original author(s). + * Copyright 2011-2015 by the original author(s). * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -110,15 +110,16 @@ public class BeanWrapper implements PersistentPropertyAccessor { try { - Method getter = property.getGetter(); - if (!property.usePropertyAccess()) { Field field = property.getField(); ReflectionUtils.makeAccessible(field); return (S) ReflectionUtils.getField(field, bean); + } - } else if (property.usePropertyAccess() && getter != null) { + Method getter = property.getGetter(); + + if (property.usePropertyAccess() && getter != null) { ReflectionUtils.makeAccessible(getter); return (S) ReflectionUtils.invokeMethod(getter, bean); diff --git a/src/main/java/org/springframework/data/mapping/model/SimpleTypeHolder.java b/src/main/java/org/springframework/data/mapping/model/SimpleTypeHolder.java index dab3f9ece..9dbf067ad 100644 --- a/src/main/java/org/springframework/data/mapping/model/SimpleTypeHolder.java +++ b/src/main/java/org/springframework/data/mapping/model/SimpleTypeHolder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011 by the original author(s). + * Copyright 2011-2015 by the original author(s). * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import java.util.Date; import java.util.HashSet; import java.util.Locale; import java.util.Set; +import java.util.concurrent.CopyOnWriteArraySet; import org.springframework.util.Assert; @@ -86,7 +87,7 @@ public class SimpleTypeHolder { public SimpleTypeHolder(Set> customSimpleTypes, boolean registerDefaults) { Assert.notNull(customSimpleTypes); - this.simpleTypes = new HashSet>(customSimpleTypes); + this.simpleTypes = new CopyOnWriteArraySet>(customSimpleTypes); if (registerDefaults) { this.simpleTypes.addAll(DEFAULTS); @@ -104,7 +105,7 @@ public class SimpleTypeHolder { Assert.notNull(customSimpleTypes); Assert.notNull(source); - this.simpleTypes = new HashSet>(customSimpleTypes); + this.simpleTypes = new CopyOnWriteArraySet>(customSimpleTypes); this.simpleTypes.addAll(source.simpleTypes); } @@ -118,12 +119,13 @@ public class SimpleTypeHolder { Assert.notNull(type); - if (Object.class.equals(type)) { + if (Object.class.equals(type) || simpleTypes.contains(type)) { return true; } for (Class clazz : simpleTypes) { if (clazz.isAssignableFrom(type)) { + simpleTypes.add(type); return true; } } diff --git a/src/main/java/org/springframework/data/util/CacheValue.java b/src/main/java/org/springframework/data/util/CacheValue.java new file mode 100644 index 000000000..3fc20fb0d --- /dev/null +++ b/src/main/java/org/springframework/data/util/CacheValue.java @@ -0,0 +1,107 @@ +/* + * Copyright 2014-2015 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.util; + +/** + * Wrapper to safely store {@literal null} values in the value cache. + * + * @author Patryk Wasik + * @author Oliver Gierke + * @author Thomas Darimont + */ +public class CacheValue { + + private static final CacheValue ABSENT = new CacheValue(null); + + private final T value; + + /** + * Creates a new {@link CacheValue} for the gven value. + * + * @param type can be {@literal null}. + */ + private CacheValue(T type) { + this.value = type; + } + + /** + * Returns the actual underlying value. + * + * @return + */ + public T getValue() { + return value; + } + + /** + * Returns whether the cached value has an actual value. + * + * @return + */ + public boolean isPresent() { + return value != null; + } + + /** + * Returns whether the cached value has the given actual value. + * + * @param value can be {@literal null}; + * @return + */ + public boolean hasValue(T value) { + return isPresent() ? this.value.equals(value) : value == null; + } + + /** + * Returns a new {@link CacheValue} for the given value. + * + * @param value can be {@literal null}. + * @return will never be {@literal null}. + */ + @SuppressWarnings("unchecked") + public static CacheValue ofNullable(T value) { + return value == null ? (CacheValue) ABSENT : new CacheValue(value); + } + + /* + * (non-Javadoc) + * @see java.lang.Object#hashCode() + */ + @Override + public int hashCode() { + return isPresent() ? 0 : value.hashCode(); + } + + /* + * (non-Javadoc) + * @see java.lang.Object#equals(java.lang.Object) + */ + @Override + public boolean equals(Object obj) { + + if (this == obj) { + return true; + } + + if (!(obj instanceof CacheValue)) { + return false; + } + + CacheValue that = (CacheValue) obj; + + return this.value == null ? false : this.value.equals(that.value); + } +} diff --git a/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java b/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java index 8fb4bc093..9072fe30a 100644 --- a/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java +++ b/src/main/java/org/springframework/data/util/ParentTypeAwareTypeInformation.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 the original author or authors. + * Copyright 2011-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,8 +20,6 @@ import java.lang.reflect.TypeVariable; import java.util.HashMap; import java.util.Map; -import org.springframework.util.ObjectUtils; - /** * Base class for {@link TypeInformation} implementations that need parent type awareness. * @@ -30,6 +28,7 @@ import org.springframework.util.ObjectUtils; public abstract class ParentTypeAwareTypeInformation extends TypeDiscoverer { private final TypeDiscoverer parent; + private int hashCode; /** * Creates a new {@link ParentTypeAwareTypeInformation}. @@ -99,6 +98,11 @@ public abstract class ParentTypeAwareTypeInformation extends TypeDiscoverer implements TypeInformation { private final Type type; private final Map, Type> typeVariableMap; private final Map> fieldTypes = new ConcurrentHashMap>(); + private final int hashCode; private boolean componentTypeResolved = false; private TypeInformation componentType; @@ -72,6 +71,7 @@ class TypeDiscoverer implements TypeInformation { this.type = type; this.typeVariableMap = typeVariableMap; + this.hashCode = 17 + (31 * type.hashCode()) + (31 * typeVariableMap.hashCode()); } /** @@ -513,10 +513,7 @@ class TypeDiscoverer implements TypeInformation { TypeDiscoverer that = (TypeDiscoverer) obj; - boolean typeEqual = nullSafeEquals(this.type, that.type); - boolean typeVariableMapEqual = nullSafeEquals(this.typeVariableMap, that.typeVariableMap); - - return typeEqual && typeVariableMapEqual; + return this.type.equals(that.type) && this.typeVariableMap.equals(that.typeVariableMap); } /* @@ -525,12 +522,6 @@ class TypeDiscoverer implements TypeInformation { */ @Override public int hashCode() { - - int result = 17; - - result += nullSafeHashCode(type); - result += nullSafeHashCode(typeVariableMap); - - return result; + return hashCode; } } diff --git a/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java b/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java index 195e0cd80..6d879be6a 100644 --- a/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java +++ b/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2011-2014 the original author or authors. + * Copyright 2011-2015 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,7 +21,7 @@ import static org.mockito.Mockito.*; import java.util.Comparator; import java.util.Iterator; -import java.util.SortedSet; +import java.util.List; import org.junit.Rule; import org.junit.Test; @@ -55,7 +55,7 @@ public class BasicPersistentEntityUnitTests> { @Rule public ExpectedException exception = ExpectedException.none(); - @Mock T property; + @Mock T property, anotherProperty; @Test public void assertInvariants() { @@ -111,8 +111,9 @@ public class BasicPersistentEntityUnitTests> { entity.addPersistentProperty(lastName); entity.addPersistentProperty(firstName); entity.addPersistentProperty(ssn); + entity.verify(); - SortedSet properties = (SortedSet) ReflectionTestUtils.getField(entity, "properties"); + List properties = (List) ReflectionTestUtils.getField(entity, "properties"); assertThat(properties.size(), is(3)); Iterator iterator = properties.iterator(); @@ -144,10 +145,11 @@ public class BasicPersistentEntityUnitTests> { MutablePersistentEntity entity = createEntity(Person.class); when(property.isIdProperty()).thenReturn(true); + when(anotherProperty.isIdProperty()).thenReturn(true); entity.addPersistentProperty(property); exception.expect(MappingException.class); - entity.addPersistentProperty(property); + entity.addPersistentProperty(anotherProperty); } /**