You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@isis.apache.org by ah...@apache.org on 2019/10/01 21:24:08 UTC
[isis] branch v2 updated: ISIS-2158: MethodRemover: using
Set instead of List
This is an automated email from the ASF dual-hosted git repository.
ahuber pushed a commit to branch v2
in repository https://gitbox.apache.org/repos/asf/isis.git
The following commit(s) were added to refs/heads/v2 by this push:
new fd4ccc6 ISIS-2158: MethodRemover: using Set<Method> instead of List<Method>
fd4ccc6 is described below
commit fd4ccc67bb7b3a1db6225c1317d52742517f9a44
Author: Andi Huber <ah...@apache.org>
AuthorDate: Tue Oct 1 23:24:00 2019 +0200
ISIS-2158: MethodRemover: using Set<Method> instead of List<Method>
- should(!?) perform better on classes with lots of methods
- also solving concurrent modification issues when stress testing
spec-loading
---
.../apache/isis/metamodel/commons/MethodUtil.java | 151 ++++++++++++---------
.../isis/metamodel/facetapi/MethodRemover.java | 9 +-
.../specloader/SpecificationLoaderDefault.java | 5 -
.../specloader/facetprocessor/FacetProcessor.java | 10 +-
.../specloader/specimpl/FacetedMethodsBuilder.java | 87 +++++++-----
.../domainmodel/SpecloaderPerformanceTest.java | 4 +-
.../publishing/PublisherServiceTest.java | 2 +-
7 files changed, 157 insertions(+), 111 deletions(-)
diff --git a/core/metamodel/src/main/java/org/apache/isis/metamodel/commons/MethodUtil.java b/core/metamodel/src/main/java/org/apache/isis/metamodel/commons/MethodUtil.java
index b04c719..798ef39 100644
--- a/core/metamodel/src/main/java/org/apache/isis/metamodel/commons/MethodUtil.java
+++ b/core/metamodel/src/main/java/org/apache/isis/metamodel/commons/MethodUtil.java
@@ -23,10 +23,13 @@ import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.Collection;
import java.util.List;
+import java.util.Set;
import java.util.function.Consumer;
import org.apache.isis.metamodel.methodutils.MethodScope;
+import lombok.val;
+
public class MethodUtil {
private MethodUtil(){}
@@ -48,13 +51,22 @@ public class MethodUtil {
* The search algorithm is as specified in
* {@link MethodUtil#findMethodIndex(List, MethodScope, String, Class, Class[])}.
*/
- public static Method removeMethod(final List<Method> methods, final MethodScope methodScope, final String name, final Class<?> returnType, final Class<?>[] paramTypes) {
- final int idx = MethodUtil.findMethodIndex(methods, methodScope, name, returnType, paramTypes);
- if (idx != -1) {
- final Method method = methods.get(idx);
- methods.set(idx, null);
- return method;
+ public static Method removeMethod(
+ final Set<Method> methods,
+ final MethodScope methodScope,
+ final String name,
+ final Class<?> returnType,
+ final Class<?>[] paramTypes) {
+
+ val methodIterator = methods.iterator();
+ while(methodIterator.hasNext()) {
+ val method = methodIterator.next();
+ if(matches(method, methodScope, name, returnType, paramTypes)){
+ methodIterator.remove();
+ return method;
+ }
}
+
return null;
}
@@ -72,53 +84,50 @@ public class MethodUtil {
* </ul>
* If the returnType is specified as null then the return type is ignored.
*/
- private static int findMethodIndex(final List<Method> methods, final MethodScope methodScope, final String name, final Class<?> returnType, final Class<?>[] paramTypes) {
- int idx = -1;
- method: for (int i = 0; i < methods.size(); i++) {
- if (methods.get(i) == null) {
- continue;
- }
+ private static boolean matches(
+ final Method method,
+ final MethodScope methodScope,
+ final String name,
+ final Class<?> returnType,
+ final Class<?>[] paramTypes) {
+
+ final int modifiers = method.getModifiers();
- final Method method = methods.get(i);
- final int modifiers = method.getModifiers();
+ // check for public modifier
+ if (!Modifier.isPublic(modifiers)) {
+ return false;
+ }
- // check for public modifier
- if (!Modifier.isPublic(modifiers)) {
- continue;
- }
+ // check for static modifier
+ if (!inScope(method, methodScope)) {
+ return false;
+ }
- // check for static modifier
- if (!inScope(method, methodScope)) {
- continue;
- }
+ // check for name
+ if (!method.getName().equals(name)) {
+ return false;
+ }
- // check for name
- if (!method.getName().equals(name)) {
- continue;
- }
+ // check for return type
+ if (returnType != null && returnType != method.getReturnType()) {
+ return false;
+ }
- // check for return type
- if (returnType != null && returnType != method.getReturnType()) {
- continue;
+ // check params (if required)
+ if (paramTypes != null) {
+ final Class<?>[] parameterTypes = method.getParameterTypes();
+ if (paramTypes.length != parameterTypes.length) {
+ return false;
}
- // check params (if required)
- if (paramTypes != null) {
- final Class<?>[] parameterTypes = method.getParameterTypes();
- if (paramTypes.length != parameterTypes.length) {
- continue;
- }
-
- for (int c = 0; c < paramTypes.length; c++) {
- if ((paramTypes[c] != null) && (paramTypes[c] != parameterTypes[c])) {
- continue method;
- }
+ for (int c = 0; c < paramTypes.length; c++) {
+ if ((paramTypes[c] != null) && (paramTypes[c] != parameterTypes[c])) {
+ return false;
}
}
- idx = i;
- break;
}
- return idx;
+
+ return true;
}
public static boolean inScope(final Method extendee, final MethodScope methodScope) {
@@ -154,36 +163,44 @@ public class MethodUtil {
* @return Method
*/
public static void removeMethods(
- final List<Method> methods,
- final MethodScope forClass,
- final String prefix,
- final Class<?> returnType,
- final boolean canBeVoid,
- final int paramCount,
+ Set<Method> methods,
+ MethodScope forClass,
+ String prefix,
+ Class<?> returnType,
+ boolean canBeVoid,
+ int paramCount,
Consumer<Method> onMatch) {
- for (int i = 0; i < methods.size(); i++) {
- final Method method = methods.get(i);
- if (method == null) {
- continue;
- }
-
- if (!inScope(method, forClass)) {
- continue;
- }
-
- final boolean goodPrefix = method.getName().startsWith(prefix);
+ methods.removeIf(method ->
+ matches(method, forClass, prefix, returnType, canBeVoid, paramCount, onMatch));
+
+ }
+
+ private static boolean matches(
+ Method method,
+ MethodScope forClass,
+ String prefix,
+ Class<?> returnType,
+ boolean canBeVoid,
+ int paramCount,
+ Consumer<Method> onMatch) {
+
+ if (!inScope(method, forClass)) {
+ return false;
+ }
- final boolean goodCount = method.getParameterTypes().length == paramCount;
- final Class<?> type = method.getReturnType();
- final boolean goodReturn = ClassExtensions.isCompatibleAsReturnType(returnType, canBeVoid, type);
+ val goodPrefix = method.getName().startsWith(prefix);
+ val goodCount = method.getParameterTypes().length == paramCount;
+ val type = method.getReturnType();
+ val goodReturn = ClassExtensions.isCompatibleAsReturnType(returnType, canBeVoid, type);
- if (goodPrefix && goodCount && goodReturn) {
- onMatch.accept(method);
- methods.set(i, null);
- }
+ if (goodPrefix && goodCount && goodReturn) {
+ onMatch.accept(method);
+ return true;
}
+ return false;
+
}
diff --git a/core/metamodel/src/main/java/org/apache/isis/metamodel/facetapi/MethodRemover.java b/core/metamodel/src/main/java/org/apache/isis/metamodel/facetapi/MethodRemover.java
index 609671c..ea9de98 100644
--- a/core/metamodel/src/main/java/org/apache/isis/metamodel/facetapi/MethodRemover.java
+++ b/core/metamodel/src/main/java/org/apache/isis/metamodel/facetapi/MethodRemover.java
@@ -48,6 +48,7 @@ public interface MethodRemover {
Consumer<Method> onRemoval
);
+ /*variant with noop consumer*/
default void removeMethods(
MethodScope methodScope,
String prefix,
@@ -55,7 +56,7 @@ public interface MethodRemover {
boolean canBeVoid,
int paramCount) {
- removeMethods(methodScope, prefix, returnType, canBeVoid, paramCount, whatever -> {});
+ removeMethods(methodScope, prefix, returnType, canBeVoid, paramCount, removedMethod -> {});
}
@@ -68,7 +69,11 @@ public interface MethodRemover {
* - whether looking for <tt>static</tt> (class) or
* instance-level methods.
*/
- void removeMethod(MethodScope methodScope, String methodName, Class<?> returnType, Class<?>[] parameterTypes);
+ void removeMethod(
+ MethodScope methodScope,
+ String methodName,
+ Class<?> returnType,
+ Class<?>[] parameterTypes);
void removeMethod(Method method);
diff --git a/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/SpecificationLoaderDefault.java b/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/SpecificationLoaderDefault.java
index ab1c1b9..6782b42 100644
--- a/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/SpecificationLoaderDefault.java
+++ b/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/SpecificationLoaderDefault.java
@@ -30,7 +30,6 @@ import org.springframework.stereotype.Service;
import org.apache.isis.commons.internal.base._Timing;
import org.apache.isis.commons.internal.collections._Lists;
-import org.apache.isis.commons.internal.debug._Probe;
import org.apache.isis.config.IsisConfiguration;
import org.apache.isis.config.registry.IsisBeanTypeRegistry;
import org.apache.isis.metamodel.facetapi.Facet;
@@ -182,9 +181,7 @@ public class SpecificationLoaderDefault implements SpecificationLoader {
});
val stopWatch = _Timing.now();
-
val cachedSpecifications = cache.snapshotSpecs();
- probe.println("cachedSpecifications " + cachedSpecifications.size());
logBefore(specificationsFromRegistry, cachedSpecifications);
@@ -281,8 +278,6 @@ public class SpecificationLoaderDefault implements SpecificationLoader {
// -- LOOKUP
- private final static _Probe probe = _Probe.unlimited().label("init");
-
@Override
public Collection<ObjectSpecification> snapshotSpecifications() {
return cache.snapshotSpecs();
diff --git a/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/facetprocessor/FacetProcessor.java b/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/facetprocessor/FacetProcessor.java
index a19d4e7..9d0e5ac 100644
--- a/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/facetprocessor/FacetProcessor.java
+++ b/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/facetprocessor/FacetProcessor.java
@@ -20,6 +20,7 @@
package org.apache.isis.metamodel.specloader.facetprocessor;
import java.lang.reflect.Method;
+import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -161,13 +162,16 @@ public class FacetProcessor {
* Delegates to all known
* {@link PropertyOrCollectionIdentifyingFacetFactory}s.
*/
- public Set<Method> findAssociationCandidateAccessors(final List<Method> methods, final Set<Method> candidates) {
+ public Set<Method> findAssociationCandidateAccessors(
+ final Collection<Method> methods,
+ final Set<Method> candidates) {
+
cachePropertyOrCollectionIdentifyingFacetFactoriesIfRequired();
- for (final Method method : methods) {
+ for (val method : methods) {
if (method == null) {
continue;
}
- for (final PropertyOrCollectionIdentifyingFacetFactory facetFactory : cachedPropertyOrCollectionIdentifyingFactories) {
+ for (val facetFactory : cachedPropertyOrCollectionIdentifyingFactories) {
if (facetFactory.isPropertyOrCollectionAccessorCandidate(method)) {
candidates.add(method);
}
diff --git a/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/specimpl/FacetedMethodsBuilder.java b/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/specimpl/FacetedMethodsBuilder.java
index 4de54c8..19592a8 100644
--- a/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/specimpl/FacetedMethodsBuilder.java
+++ b/core/metamodel/src/main/java/org/apache/isis/metamodel/specloader/specimpl/FacetedMethodsBuilder.java
@@ -20,12 +20,14 @@
package org.apache.isis.metamodel.specloader.specimpl;
import java.lang.reflect.Method;
-import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
import org.apache.isis.applib.annotation.Action;
import org.apache.isis.commons.exceptions.IsisException;
@@ -60,12 +62,17 @@ public class FacetedMethodsBuilder {
private static final String GET_PREFIX = "get";
private static final String IS_PREFIX = "is";
+ /* thread-safety ... make sure every methodsRemaining access is synchronized! */
private static final class FacetedMethodsMethodRemover implements MethodRemover {
- private final List<Method> methods;
+ //private final Object $lock = new Object();
+
+ private final Set<Method> methodsRemaining;
- private FacetedMethodsMethodRemover(final Class<?> introspectedClass, final List<Method> methods) {
- this.methods = methods;
+ private FacetedMethodsMethodRemover(final Class<?> introspectedClass, Method[] methods) {
+ this.methodsRemaining = Stream.of(methods)
+ .collect(Collectors.toCollection(_Sets::newConcurrentHashSet));
+ //.collect(Collectors.toCollection(_Sets::newHashSet));
}
@Override
@@ -75,7 +82,9 @@ public class FacetedMethodsBuilder {
Class<?> returnType,
Class<?>[] parameterTypes) {
- MethodUtil.removeMethod(methods, methodScope, methodName, returnType, parameterTypes);
+ //synchronized($lock) {
+ MethodUtil.removeMethod(methodsRemaining, methodScope, methodName, returnType, parameterTypes);
+ //}
}
@Override
@@ -87,30 +96,39 @@ public class FacetedMethodsBuilder {
int paramCount,
Consumer<Method> onRemoval) {
- MethodUtil.removeMethods(methods, methodScope, prefix, returnType, canBeVoid, paramCount, onRemoval);
+ //synchronized($lock) {
+ MethodUtil.removeMethods(methodsRemaining, methodScope, prefix, returnType, canBeVoid, paramCount, onRemoval);
+ //}
}
@Override
- public void removeMethod(final Method method) {
- if (method == null) {
+ public void removeMethod(Method method) {
+ if(method==null) {
return;
}
- for (int i = 0; i < methods.size(); i++) {
- if (methods.get(i) == null) {
- continue;
- }
- if (methods.get(i).equals(method)) {
- methods.set(i, null);
- }
- }
+ //synchronized($lock) {
+ methodsRemaining.remove(method);
+ //}
}
+ void removeIf(Predicate<Method> matcher) {
+ //synchronized($lock) {
+ methodsRemaining.removeIf(matcher);
+ //}
+ }
+
+ void acceptRemaining(Consumer<Set<Method>> consumer) {
+ //synchronized($lock) {
+ consumer.accept(methodsRemaining);
+ //}
+ }
+
}
private final ObjectSpecificationAbstract spec;
private final Class<?> introspectedClass;
- private final List<Method> methods;
+ // private final Set<Method> methodsRemaining;
private List<FacetedMethod> associationFacetMethods;
private List<FacetedMethod> actionFacetedMethods;
@@ -134,15 +152,16 @@ public class FacetedMethodsBuilder {
public FacetedMethodsBuilder(
final ObjectSpecificationAbstract spec,
final FacetedMethodsBuilderContext facetedMethodsBuilderContext) {
+
if (log.isDebugEnabled()) {
log.debug("creating JavaIntrospector for {}", spec.getFullIdentifier());
}
this.spec = spec;
this.introspectedClass = spec.getCorrespondingClass();
- this.methods = Arrays.asList(introspectedClass.getMethods());
-
- this.methodRemover = new FacetedMethodsMethodRemover(introspectedClass, methods);
+
+ val methodsRemaining = introspectedClass.getMethods();
+ this.methodRemover = new FacetedMethodsMethodRemover(introspectedClass, methodsRemaining);
this.facetProcessor = facetedMethodsBuilderContext.facetProcessor;
this.specificationLoader = facetedMethodsBuilderContext.specificationLoader;
@@ -220,7 +239,13 @@ public class FacetedMethodsBuilder {
if (log.isDebugEnabled()) {
log.debug("introspecting {}: properties and collections", getClassName());
}
- final Set<Method> associationCandidateMethods = getFacetProcessor().findAssociationCandidateAccessors(methods, new HashSet<Method>());
+
+ final Set<Method> associationCandidateMethods = new HashSet<Method>();
+
+ methodRemover.acceptRemaining(methodsRemaining->{
+ getFacetProcessor()
+ .findAssociationCandidateAccessors(methodsRemaining, associationCandidateMethods);
+ });
// Ensure all return types are known
final Set<Class<?>> typesToLoad = _Sets.newHashSet();
@@ -348,17 +373,14 @@ public class FacetedMethodsBuilder {
log.debug(" looking for action methods");
}
- for (int i = 0; i < methods.size(); i++) {
- final Method method = methods.get(i);
- if (method == null) {
- continue;
- }
- final FacetedMethod actionPeer = findActionFacetedMethod(methodScope, method);
+ methodRemover.removeIf(method->{
+ val actionPeer = findActionFacetedMethod(methodScope, method);
if (actionPeer != null) {
- methods.set(i, null);
actionFacetedMethods.add(actionPeer);
+ return true;
}
- }
+ return false;
+ });
}
@@ -492,7 +514,7 @@ public class FacetedMethodsBuilder {
/**
* Searches for all methods matching the prefix and returns them, also
- * removing it from the {@link #methods array of methods} if found.
+ * removing it from the {@link #methodsRemaining array of methods} if found.
* @param onMatch
*/
private void findAndRemovePrefixedMethods(
@@ -503,7 +525,10 @@ public class FacetedMethodsBuilder {
final int paramCount,
Consumer<Method> onMatch) {
- MethodUtil.removeMethods(methods, methodScope, prefix, returnType, canBeVoid, paramCount, onMatch);
+ methodRemover.acceptRemaining(methodsRemaining->{
+ MethodUtil.removeMethods(methodsRemaining, methodScope, prefix, returnType, canBeVoid, paramCount, onMatch);
+ });
+
}
// ////////////////////////////////////////////////////////////////////////////
diff --git a/examples/smoketests/src/test/java/org/apache/isis/testdomain/domainmodel/SpecloaderPerformanceTest.java b/examples/smoketests/src/test/java/org/apache/isis/testdomain/domainmodel/SpecloaderPerformanceTest.java
index 94a0202..157f477 100644
--- a/examples/smoketests/src/test/java/org/apache/isis/testdomain/domainmodel/SpecloaderPerformanceTest.java
+++ b/examples/smoketests/src/test/java/org/apache/isis/testdomain/domainmodel/SpecloaderPerformanceTest.java
@@ -63,9 +63,9 @@ class SpecloaderPerformanceTest {
@Test //under constr.
void repeatedSpecloading() {
- config.getReflector().getIntrospector().setParallelize(true);
+ config.getReflector().getIntrospector().setParallelize(false);
- for(int i=0; i<100; ++i) {
+ for(int i=0; i<40; ++i) {
specificationLoader.shutdown();
specificationLoader.init();
}
diff --git a/examples/smoketests/src/test/java/org/apache/isis/testdomain/publishing/PublisherServiceTest.java b/examples/smoketests/src/test/java/org/apache/isis/testdomain/publishing/PublisherServiceTest.java
index 7ae52d1..bbc9de4 100644
--- a/examples/smoketests/src/test/java/org/apache/isis/testdomain/publishing/PublisherServiceTest.java
+++ b/examples/smoketests/src/test/java/org/apache/isis/testdomain/publishing/PublisherServiceTest.java
@@ -120,7 +120,7 @@ class PublisherServiceTest {
future.get(1000, TimeUnit.SECONDS);
// then - after the commit
- assertEquals("publishedObjects=created=0,deleted=1,loaded=0,updated=2,modified=1,",
+ assertEquals("publishedObjects=created=0,deleted=1,loaded=1,updated=2,modified=1,",
publisherService.getHistory());
}