Use shallow clone while expanding definitions
When walking ancestors, a lot more node refreshing is done. Refreshing
nodes often involves expanding the task definitions which involves
copying the original definition. Reduce the time spent doing these
copies drastically by using a shallow clone instead of the previous deep
copy. This has been safe to do for some time because of the COW policy
which is being used during expansion and preloading. Additionally,
accidental overwrites have also been safeguarded against for some time
now by using unmodifiable collections in the definitions, and this
change adds two additional redudant safeguards. To be consistent and to
prevent future issues with NamesFactories, use a shallow clone for them
also.
The performance difference in the case of a task.config which walks all
dependencies for a change when run with status:open --no-limit
--task--applicable is about 1/3 faster:
Before this change: 24m13s 24m36s 22m48s 23m36s 24m22s
After this change: 15m14s 15m46s 17m37s 15m24s 15m11s
Change-Id: I2f78b42c1fa68f2751ac04e26f6ec4d5989db585
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Copier.java b/src/main/java/com/googlesource/gerrit/plugins/task/Copier.java
index 2c0ccad..10f4048 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Copier.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Copier.java
@@ -14,44 +14,19 @@
package com.googlesource.gerrit.plugins.task;
-import com.google.common.primitives.Primitives;
import java.lang.reflect.Field;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
public class Copier {
- protected static <T> void deepCopyDeclaredFields(
- Class<T> cls,
- T from,
- T to,
- boolean includeInaccessible,
- Collection<Class<?>> copyReferenceOnly) {
+ protected static <T> void shallowCopyDeclaredFields(
+ Class<T> cls, T from, T to, boolean includeInaccessible) {
for (Field field : cls.getDeclaredFields()) {
try {
if (includeInaccessible) {
field.setAccessible(true);
}
- Class<?> fieldCls = field.getType();
Object val = field.get(from);
- if (field.getType().isPrimitive()
- || Primitives.isWrapperType(fieldCls)
- || (val instanceof String)
- || val == null
- || copyReferenceOnly.contains(fieldCls)) {
+ if (!field.getName().equals("this$0")) { // Can't copy internal final field
field.set(to, val);
- } else if (val instanceof List) {
- List<?> list = List.class.cast(val);
- field.set(to, new ArrayList<>(list));
- } else if (val instanceof Map) {
- Map<?, ?> map = Map.class.cast(val);
- field.set(to, new HashMap<>(map));
- } else if (field.getName().equals("this$0")) { // Can't copy internal final field
- } else {
- throw new RuntimeException(
- "Don't know how to deep copy " + fieldValueToString(field, val));
}
} catch (IllegalAccessException e) {
if (includeInaccessible) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java b/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java
index efafd57..8369d67 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java
@@ -14,9 +14,58 @@
package com.googlesource.gerrit.plugins.task;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.Optional;
import java.util.function.Function;
public class CopyOnWrite<T> {
+ public static class CloneOnWrite<C extends Cloneable> extends CopyOnWrite<C> {
+ public CloneOnWrite(C cloneable) {
+ super(cloneable, copier(cloneable));
+ }
+ }
+
+ public static <C extends Cloneable> Function<C, C> copier(C cloneable) {
+ return c -> clone(c);
+ }
+
+ @SuppressWarnings("unchecked")
+ public static <C extends Cloneable> C clone(C cloneable) {
+ try {
+ for (Class<?> cls = cloneable.getClass(); cls != null; cls = cls.getSuperclass()) {
+ Optional<Method> optional = getOptionalDeclaredMethod(cls, "clone");
+ if (optional.isPresent()) {
+ Method clone = optional.get();
+ clone.setAccessible(true);
+ return (C) cloneable.getClass().cast(clone.invoke(cloneable));
+ }
+ }
+ throw new RuntimeException("Cannot find clone() method");
+ } catch (SecurityException
+ | IllegalAccessException
+ | IllegalArgumentException
+ | InvocationTargetException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * A faster getDeclaredMethod() without exceptions. The original apparently does a linear search
+ * anyway, and it is significantly slower when it throws NoSuchMethodExceptions.
+ */
+ public static Optional<Method> getOptionalDeclaredMethod(
+ Class<?> cls, String name, Class<?>... parameterTypes) {
+ for (Method method : cls.getDeclaredMethods()) {
+ if (method.getName().equals(name)
+ && Arrays.equals(method.getParameterTypes(), parameterTypes)) {
+ return Optional.of(method);
+ }
+ }
+ return Optional.empty();
+ }
+
protected Function<T, T> copier;
protected T original;
protected T copy;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
index 4a04e2e..952b9a3 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
@@ -60,7 +60,7 @@
public Properties(TaskTree.Node node, Task origTask) {
this.node = node;
this.origTask = origTask;
- task = new CopyOnWrite<>(origTask, t -> origTask.config.new Task(t));
+ task = new CopyOnWrite.CloneOnWrite<>(origTask);
}
/** Use to expand properties specifically for Tasks. */
@@ -103,10 +103,7 @@
/** Use to expand properties specifically for NamesFactories. */
public NamesFactory getNamesFactory(NamesFactory namesFactory) {
- return expander.expand(
- namesFactory,
- nf -> namesFactory.config.new NamesFactory(nf),
- ImmutableSet.of(TaskConfig.KEY_TYPE));
+ return expander.expand(namesFactory, ImmutableSet.of(TaskConfig.KEY_TYPE));
}
protected Function<String, String> getParentMapper() {
@@ -281,6 +278,14 @@
* Returns expanded object if property found in the Strings in the object's Fields (except the
* excluded ones). Returns same object if no expansions occurred.
*/
+ public <C extends Cloneable> C expand(C object, Set<String> excludedFieldNames) {
+ return expand(new CopyOnWrite.CloneOnWrite<>(object), excludedFieldNames);
+ }
+
+ /**
+ * Returns expanded object if property found in the Strings in the object's Fields (except the
+ * excluded ones). Returns same object if no expansions occurred.
+ */
public <T> T expand(T object, Function<T, T> copier, Set<String> excludedFieldNames) {
return expand(new CopyOnWrite<>(object, copier), excludedFieldNames);
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
index 9236db7..b4e79e0 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -14,13 +14,11 @@
package com.googlesource.gerrit.plugins.task;
-import com.google.common.collect.Sets;
import com.google.gerrit.common.Container;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.server.git.meta.AbstractVersionedMetaData;
import java.util.ArrayList;
import java.util.Arrays;
-import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@@ -94,7 +92,7 @@
protected TaskBase(TaskBase base) {
this(base.subSection);
- Copier.deepCopyDeclaredFields(TaskBase.class, base, this, false, copyOnlyReferencesFor());
+ Copier.shallowCopyDeclaredFields(TaskBase.class, base, this, false);
}
protected TaskBase(SubSectionKey s) {
@@ -102,7 +100,7 @@
}
}
- public class Task extends TaskBase {
+ public class Task extends TaskBase implements Cloneable {
public final TaskKey key;
public Task(SubSectionKey s, boolean isVisible, boolean isMasqueraded) {
@@ -110,14 +108,6 @@
key = TaskKey.create(s);
}
- public Task(Task task) {
- super(task);
- // Despite being copied in Copier.deepCopyDeclaredFields this
- // is needed to avoid the final variable initialization warning.
- this.key = task.key;
- Copier.deepCopyDeclaredFields(Task.class, task, this, false, copyOnlyReferencesFor());
- }
-
public Task(TasksFactory tasks, String name) {
super(tasks);
key = TaskKey.create(tasks.subSection, name);
@@ -156,7 +146,7 @@
}
}
- public class NamesFactory extends SubSection {
+ public class NamesFactory extends SubSection implements Cloneable {
public String changes;
public List<String> names;
public String type;
@@ -167,11 +157,6 @@
names = getStringList(s, KEY_NAME);
type = getString(s, KEY_TYPE, null);
}
-
- public NamesFactory(NamesFactory n) {
- super(n.subSection);
- Copier.deepCopyDeclaredFields(NamesFactory.class, n, this, false, copyOnlyReferencesFor());
- }
}
public class External extends SubSection {
@@ -287,7 +272,7 @@
for (String name : names) {
valueByName.put(name, getString(s, name));
}
- return valueByName;
+ return Collections.unmodifiableMap(valueByName);
}
protected Set<String> getMatchingNames(SubSectionKey s, String match) {
@@ -297,7 +282,7 @@
matched.add(name);
}
}
- return matched;
+ return Collections.unmodifiableSet(matched);
}
protected Set<String> getNames(SubSectionKey s) {
@@ -321,8 +306,4 @@
protected SubSectionKey subSectionKey(String section, String subSection) {
return SubSectionKey.create(file, section, subSection);
}
-
- protected Collection<Class<?>> copyOnlyReferencesFor() {
- return Sets.newHashSet(TaskKey.class, SubSectionKey.class);
- }
}
