From 2b4ef1a55589466c87c2f30432f504e7b00a2438 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Tue, 26 Sep 2017 14:58:56 +0200 Subject: [PATCH] DATACMNS-1175 - Remove argument array caching from EntityInstantiators. We no longer cache argument arrays in our EntityInstantiators to prevent changes to shared mutable state caused by reentrant calls. Previously, a re-entrant call requesting an argument array of the same size as a previous call in the call stack reused the same array instance. Changes to this shared mutable state by multiple invocations caused an invalid state rendering wrong parameters for object instantiation. Removing the caching and only reusing an empty array for zero-arg constructors is the only safe approach for now. Re-instantiation of object allocations results in a higher GC pressure but guarantee side effect-free instantiation and should be on-par with previous versions performance profile. Original pull request: #247. --- .../ClassGeneratingEntityInstantiator.java | 35 ++----------- ...tlinClassGeneratingEntityInstantiator.java | 6 +-- .../convert/ReflectionEntityInstantiator.java | 17 +------ ...GeneratingEntityInstantiatorUnitTests.java | 49 ++++++++++++++++++- 4 files changed, 54 insertions(+), 53 deletions(-) diff --git a/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java index 59073f2a6..144b3e005 100644 --- a/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/ClassGeneratingEntityInstantiator.java @@ -55,18 +55,7 @@ import org.springframework.util.ClassUtils; */ public class ClassGeneratingEntityInstantiator implements EntityInstantiator { - private static final int ARG_CACHE_SIZE = 100; - - private static final ThreadLocal OBJECT_POOL = ThreadLocal.withInitial(() -> { - - Object[][] cached = new Object[ARG_CACHE_SIZE][]; - - for (int i = 0; i < ARG_CACHE_SIZE; i++) { - cached[i] = new Object[i]; - } - - return cached; - }); + private static final Object[] EMPTY_ARGS = new Object[0]; private final ObjectInstantiatorClassGenerator generator; @@ -170,30 +159,14 @@ public class ClassGeneratingEntityInstantiator implements EntityInstantiator { } /** - * Allocates an object array for instance creation. This method uses the argument array cache if possible. + * Allocates an object array for instance creation. * * @param argumentCount * @return * @since 2.0 - * @see #ARG_CACHE_SIZE */ static Object[] allocateArguments(int argumentCount) { - return argumentCount < ARG_CACHE_SIZE ? OBJECT_POOL.get()[argumentCount] : new Object[argumentCount]; - } - - /** - * Deallocates an object array used for instance creation. Parameters are cleared if the array was cached. - * - * @param argumentCount - * @return - * @since 2.0 - * @see #ARG_CACHE_SIZE - */ - static void deallocateArguments(Object[] params) { - - if (params.length != 0 && params.length < ARG_CACHE_SIZE) { - Arrays.fill(params, null); - } + return argumentCount == 0 ? EMPTY_ARGS : new Object[argumentCount]; } /** @@ -250,8 +223,6 @@ public class ClassGeneratingEntityInstantiator implements EntityInstantiator { return (T) instantiator.newInstance(params); } catch (Exception e) { throw new MappingInstantiationException(entity, Arrays.asList(params), e); - } finally { - deallocateArguments(params); } } diff --git a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java index c4da91e91..1bdb23a3a 100644 --- a/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/KotlinClassGeneratingEntityInstantiator.java @@ -237,11 +237,7 @@ public class KotlinClassGeneratingEntityInstantiator extends ClassGeneratingEnti params[userParameterCount + i] = defaulting[i]; } - try { - return (T) instantiator.newInstance(params); - } finally { - deallocateArguments(params); - } + return (T) instantiator.newInstance(params); } } } diff --git a/src/main/java/org/springframework/data/convert/ReflectionEntityInstantiator.java b/src/main/java/org/springframework/data/convert/ReflectionEntityInstantiator.java index 6c53f0f33..6fc71a6b4 100644 --- a/src/main/java/org/springframework/data/convert/ReflectionEntityInstantiator.java +++ b/src/main/java/org/springframework/data/convert/ReflectionEntityInstantiator.java @@ -40,18 +40,7 @@ public enum ReflectionEntityInstantiator implements EntityInstantiator { INSTANCE; - private final int ARG_CACHE_SIZE = 100; - - private final ThreadLocal objectPool = ThreadLocal.withInitial(() -> { - - Object[][] cached = new Object[ARG_CACHE_SIZE][]; - - for (int i = 0; i < ARG_CACHE_SIZE; i++) { - cached[i] = new Object[i]; - } - - return cached; - }); + private static final Object[] EMPTY_ARGS = new Object[0]; @SuppressWarnings("unchecked") public , P extends PersistentProperty

> T createInstance(E entity, @@ -80,7 +69,7 @@ public enum ReflectionEntityInstantiator implements EntityInstantiator { } int parameterCount = constructor.getConstructor().getParameterCount(); - Object[] params = parameterCount < ARG_CACHE_SIZE ? objectPool.get()[parameterCount] : new Object[parameterCount]; + Object[] params = parameterCount == 0 ? EMPTY_ARGS : new Object[parameterCount]; int i = 0; for (Parameter parameter : constructor.getParameters()) { params[i++] = provider.getParameterValue(parameter); @@ -90,8 +79,6 @@ public enum ReflectionEntityInstantiator implements EntityInstantiator { return BeanUtils.instantiateClass(constructor.getConstructor(), params); } catch (BeanInstantiationException e) { throw new MappingInstantiationException(entity, new ArrayList<>(Arrays.asList(params)), e); - } finally { - Arrays.fill(params, null); } } } diff --git a/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java b/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java index 5a578b875..242b1a701 100755 --- a/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java +++ b/src/test/java/org/springframework/data/convert/ClassGeneratingEntityInstantiatorUnitTests.java @@ -147,6 +147,40 @@ public class ClassGeneratingEntityInstantiatorUnitTests

outer = new BasicPersistentEntity<>(from(SampleWithReference.class)); + PersistentEntity inner = new BasicPersistentEntity<>(from(Sample.class)); + + doReturn(2L, "FOO").when(provider).getParameterValue(any(Parameter.class)); + + ParameterValueProvider

recursive = new ParameterValueProvider

() { + + @Override + public T getParameterValue(Parameter parameter) { + + if (parameter.getName().equals("id")) { + return (T) Long.valueOf(1); + } + + if (parameter.getName().equals("sample")) { + return (T) instance.createInstance(inner, provider); + } + + throw new UnsupportedOperationException(parameter.getName()); + } + }; + + SampleWithReference reference = this.instance.createInstance(outer, recursive); + + assertThat(reference.id).isEqualTo(1L); + assertThat(reference.sample).isNotNull(); + assertThat(reference.sample.id).isEqualTo(2L); + assertThat(reference.sample.name).isEqualTo("FOO"); + } + @Test // DATACMNS-578, DATACMNS-1126 public void instantiateObjCtorDefault() { @@ -154,7 +188,8 @@ public class ClassGeneratingEntityInstantiatorUnitTests

assertThat(this.instance.createInstance(entity, provider)).isInstanceOf(ObjCtorDefault.class)); + IntStream.range(0, 2) + .forEach(i -> assertThat(this.instance.createInstance(entity, provider)).isInstanceOf(ObjCtorDefault.class)); } @Test // DATACMNS-578, DATACMNS-1126 @@ -283,6 +318,18 @@ public class ClassGeneratingEntityInstantiatorUnitTests