From 50a3d9dae8da664f4bf31f0193503d8150637ddb Mon Sep 17 00:00:00 2001 From: AndreasTu Date: Sun, 3 Sep 2023 12:40:28 +0200 Subject: [PATCH] Use multiple locks in ByteBuddyMockFactory Changed locking scheme in ByteBuddyMockFactory from a single global lock CACHE to cacheLocks with size 16. The used TypeCachingLock from cacheLocks depends on the hashcode of TypeCache.SimpleKey, which aggregates the types and settings to mock. The old global CACHE lock did block Threads regardless, if they try to mock the same type or not. This is a similar fix as in Mockito: https://github.com/mockito/mockito/pull/3095 --- .../mock/runtime/ByteBuddyMockFactory.java | 77 ++++++++- .../ByteBuddyMockFactoryConcurrentSpec.groovy | 146 ++++++++++++++++++ .../runtime/ByteBuddyTestClassLoader.java | 55 +++++++ 3 files changed, 275 insertions(+), 3 deletions(-) create mode 100644 spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyMockFactoryConcurrentSpec.groovy create mode 100644 spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyTestClassLoader.java diff --git a/spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java b/spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java index 030b835b08..f4ed2ae0b6 100644 --- a/spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java +++ b/spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java @@ -1,3 +1,19 @@ +/* + * Copyright 2023 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 + * + * https://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.spockframework.mock.runtime; import net.bytebuddy.ByteBuddy; @@ -13,24 +29,56 @@ import net.bytebuddy.implementation.MethodDelegation; import net.bytebuddy.implementation.bind.annotation.Morph; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.VisibleForTesting; import org.spockframework.mock.ISpockMockObject; import org.spockframework.mock.codegen.Target; import org.spockframework.util.Nullable; import java.lang.reflect.Method; import java.util.List; +import java.util.concurrent.Callable; import java.util.concurrent.ThreadLocalRandom; import static net.bytebuddy.matcher.ElementMatchers.any; import static net.bytebuddy.matcher.ElementMatchers.none; class ByteBuddyMockFactory { + /** + * The size of the {@link #cacheLocks}. + *

Caution: This must match the {@link #CACHE_LOCK_MASK}. + */ + private static final int CACHE_LOCK_SIZE = 16; + + /** + * The mask to use to mask out the {@link TypeCache.SimpleKey#hashCode()} to find the {@link #cacheLocks}. + *

Caution: this must match the bits of the {@link #CACHE_LOCK_SIZE}. + */ + private static final int CACHE_LOCK_MASK = 0x0F; + + /** + * This array contains {@link TypeCachingLock} instances, which are used as java monitor locks for + * {@link TypeCache#findOrInsert(ClassLoader, Object, Callable, Object)}. + * The {@code cacheLocks} spreads the lock to acquire over multiple locks instead of using a single lock + * {@code CACHE} for all {@code TypeCache.SimpleKeys}. + * + *

Note: We can't simply use the mockedType class lock as a lock, + * because the {@link TypeCache.SimpleKey}, will be the same for different {@code mockTypes + additionalInterfaces}. + * See the hashcode implementation of the {@code TypeCache.SimpleKey}, which has {@code Set} semantics. + */ + private static final TypeCachingLock[] cacheLocks; private static final TypeCache CACHE = new TypeCache.WithInlineExpunction<>(TypeCache.Sort.SOFT); private static final Class CODEGEN_TARGET_CLASS = Target.class; private static final String CODEGEN_PACKAGE = CODEGEN_TARGET_CLASS.getPackage().getName(); + static { + cacheLocks = new TypeCachingLock[CACHE_LOCK_SIZE]; + for (int i = 0; i < CACHE_LOCK_SIZE; i++) { + cacheLocks[i] = new TypeCachingLock(); + } + } + static Object createMock(final Class type, final List> additionalInterfaces, @Nullable List constructorArgs, @@ -38,8 +86,10 @@ static Object createMock(final Class type, final ClassLoader classLoader, boolean useObjenesis) { + TypeCache.SimpleKey key = new TypeCache.SimpleKey(type, additionalInterfaces); + Class enhancedType = CACHE.findOrInsert(classLoader, - new TypeCache.SimpleKey(type, additionalInterfaces), + key, () -> { String typeName = type.getName(); Class targetClass = type; @@ -68,14 +118,33 @@ static Object createMock(final Class type, .make() .load(classLoader, strategy) .getLoaded(); - }, CACHE); + }, getCacheLockForKey(key)); Object proxy = MockInstantiator.instantiate(type, enhancedType, constructorArgs, useObjenesis); ((ByteBuddyInterceptorAdapter.InterceptorAccess) proxy).$spock_set(interceptor); return proxy; } - // This methods and the ones it calls are inspired by similar code in Mockito's SubclassBytecodeGenerator + /** + * Returns a {@link TypeCachingLock}, which locks the {@link TypeCache#findOrInsert(ClassLoader, Object, Callable, Object)}. + * + * @param key the key to lock + * @return the {@link TypeCachingLock} to use to lock the {@link TypeCache} + */ + private static TypeCachingLock getCacheLockForKey(TypeCache.SimpleKey key) { + int hashCode = key.hashCode(); + // Try to spread some higher bits with XOR to lower bits, because we only use lower bits. + hashCode = hashCode ^ (hashCode >>> 16); + int index = hashCode & CACHE_LOCK_MASK; + return cacheLocks[index]; + } + + @VisibleForTesting + static void clearAllCaches() { + CACHE.clear(); + } + + // This method and the ones it calls are inspired by similar code in Mockito's SubclassBytecodeGenerator private static boolean shouldLoadIntoCodegenPackage(Class type) { return isComingFromJDK(type) || isComingFromSignedJar(type) || isComingFromSealedPackage(type); } @@ -112,4 +181,6 @@ private static ClassLoadingStrategy determineBestClassLoadingStrate return ClassLoadingStrategy.Default.WRAPPER; } + private static final class TypeCachingLock { + } } diff --git a/spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyMockFactoryConcurrentSpec.groovy b/spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyMockFactoryConcurrentSpec.groovy new file mode 100644 index 0000000000..917e8e4c84 --- /dev/null +++ b/spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyMockFactoryConcurrentSpec.groovy @@ -0,0 +1,146 @@ +/* + * Copyright 2023 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 + * + * https://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.spockframework.mock.runtime + +import groovy.transform.Canonical +import spock.lang.Isolated +import spock.lang.Shared +import spock.lang.Specification +import spock.lang.Timeout +import spock.lang.Unroll + +import java.util.concurrent.CompletableFuture +import java.util.concurrent.Phaser +import java.util.concurrent.TimeUnit +import java.util.function.Function + +// We need to run in isolation, because the ByteBuddyMockFactory is static and we interfere with the cache +// In the future we should change the ByteBuddyMockFactory to be an instance instead of static API. +@Isolated +class ByteBuddyMockFactoryConcurrentSpec extends Specification { + private static final String IfA = "IfA" + private static final String IfB = "IfB" + private static final String IfC = "IfC" + @Shared + final IProxyBasedMockInterceptor interceptor = Stub() + + // Just to be save to abort, normally the tests run in 2 secs. + @Timeout(40) + @Unroll("#test") + def "cacheLockingStressTest"(String test, MockSpec mockSpecA, MockSpec mockSpecB) { + setup: + int iterations = 5_000 + def tempClassLoader = new ByteBuddyTestClassLoader() + MockFeatures featA = toMockFeatures(mockSpecA, tempClassLoader) + MockFeatures featB = toMockFeatures(mockSpecB, tempClassLoader) + + Phaser phaser = new Phaser(4) + Function> runCode = { Runnable code -> + CompletableFuture.runAsync({ + phaser.arriveAndAwaitAdvance() + try { + for (int i = 0; i < iterations; i++) { + code.run() + } + } finally { + phaser.arrive() + } + }) + } + when: + def mockFeatAFuture = runCode.apply { + Class mockClass = mockClass(tempClassLoader, featA) + assertValidMockClass(featA, mockClass, tempClassLoader) + } + + def mockFeatBFuture = runCode.apply { + Class mockClass = mockClass(tempClassLoader, featB) + assertValidMockClass(featB, mockClass, tempClassLoader) + } + + def cacheFuture = runCode.apply(ByteBuddyMockFactory::clearAllCaches) + + phaser.arriveAndAwaitAdvance() + // Wait for test to end + int phase = phaser.arrive() + try { + phaser.awaitAdvanceInterruptibly(phase, 30, TimeUnit.SECONDS) + } finally { + // Collect exceptions from the futures, to make issues visible. + mockFeatAFuture.getNow(null) + mockFeatBFuture.getNow(null) + cacheFuture.getNow(null) + } + then: + noExceptionThrown() + + where: + test | mockSpecA | mockSpecB + "same hashcode different mockType" | mockSpec(IfA, IfB) | mockSpec(IfB, IfA) + "same hashcode same mockType" | mockSpec(IfA) | mockSpec(IfA) + "different hashcode different interfaces" | mockSpec(IfA, IfB) | mockSpec(IfB, IfC) + "unrelated classes" | mockSpec(IfA) | mockSpec(IfB) + } + + private Class mockClass(ClassLoader cl, MockFeatures feature) { + return ByteBuddyMockFactory.createMock( + feature.mockType, + feature.interfaces, + null, + interceptor, + cl, + false) + .getClass() + } + + private static MockSpec mockSpec(String mockedType, String... interfaces) { + return new MockSpec(mockedType, Arrays.asList(interfaces)) + } + + private void assertValidMockClass(MockFeatures mockFeature, Class mockClass, ClassLoader classLoader) { + assert mockClass.classLoader == classLoader + assert mockFeature.mockType.isAssignableFrom(mockClass) + for (Class anInterface : mockFeature.interfaces) { + assert anInterface.isAssignableFrom(mockClass) + } + } + + MockFeatures toMockFeatures(MockSpec mockFeaturesString, ByteBuddyTestClassLoader classLoader) { + def mockType = classLoader.defineInterface(mockFeaturesString.mockType) + def interfaces = mockFeaturesString.interfaces.collect { classLoader.defineInterface(it) } + return new MockFeatures(mockType, interfaces) + } + + /** + * Class holding the loaded classes to mock. + */ + @Canonical + private static class MockFeatures { + final Class mockType + final List> interfaces + } + + /** + * Class holding the class names to mock. + * Which will be converted into a {@link MockFeatures} during test. + */ + @Canonical + private static class MockSpec { + final String mockType + final List interfaces + } +} diff --git a/spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyTestClassLoader.java b/spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyTestClassLoader.java new file mode 100644 index 0000000000..371c303586 --- /dev/null +++ b/spock-specs/src/test/groovy/org/spockframework/mock/runtime/ByteBuddyTestClassLoader.java @@ -0,0 +1,55 @@ +/* + * Copyright 2023 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 + * + * https://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.spockframework.mock.runtime; + +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; + +import java.util.HashMap; +import java.util.Map; + +final class ByteBuddyTestClassLoader extends ClassLoader { + private final Map> cache = new HashMap<>(); + + private final ClassLoadingStrategy loadingStrategy = (loader, types) -> { + Map> result = new HashMap<>(); + for (Map.Entry entry : types.entrySet()) { + TypeDescription description = entry.getKey(); + byte[] bytes = entry.getValue(); + Class clazz = defineClass(description.getName(), bytes, 0, bytes.length); + result.put(description, clazz); + } + return result; + }; + + /** + * Defines an empty interface with the passed {@code node}. + * + * @param name the name of the interface + * @return the loaded {@code Class} + */ + synchronized Class defineInterface(String name) { + //noinspection resource + return cache.computeIfAbsent(name, nameKey -> new ByteBuddy() + .makeInterface() + .name(nameKey) + .make() + .load(this, loadingStrategy) + .getLoaded()); + } +}