Only copy task NamesFactories when modified
Use a COW model for NamesFactories as an optimization, and to pave the
way for many other optimizations. Although currently copying
NamesFactory does not have a measurable impact on the performance of
tasks, it may eventually. Additionally, being able to detect that a
change was made to a NamesFactory makes it possible to optimize
refreshing a TaskTree.Node which is needed to correct Node behavior when
evaluating subsequent changes.
The NamesFactory and Expander calling interfaces have been adapted to
more easily enable COW processing, and this nicely helps illustrate the
NamesFactory dependency on task properties better.
Change-Id: I1406a193edf236152fa197d3af5356a55b307730
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java b/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java
new file mode 100644
index 0000000..efafd57
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/CopyOnWrite.java
@@ -0,0 +1,43 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// 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 com.googlesource.gerrit.plugins.task;
+
+import java.util.function.Function;
+
+public class CopyOnWrite<T> {
+ protected Function<T, T> copier;
+ protected T original;
+ protected T copy;
+
+ public CopyOnWrite(T original, Function<T, T> copier) {
+ this.original = original;
+ this.copier = copier;
+ }
+
+ public T getOriginal() {
+ return original;
+ }
+
+ public T getForRead() {
+ return copy != null ? copy : original;
+ }
+
+ public T getForWrite() {
+ if (copy == null) {
+ copy = copier.apply(original);
+ }
+ return 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 b90ca14..8f5e1fd 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
@@ -18,15 +18,16 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
+import com.googlesource.gerrit.plugins.task.TaskConfig.NamesFactory;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
-import java.util.ListIterator;
import java.util.Map;
import java.util.Set;
+import java.util.function.Function;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -48,13 +49,13 @@
expandFieldValues(definition, Collections.emptySet());
}
- }
- /** Use to expand properties specifically for NamesFactories. */
- public static class NamesFactory extends Expander {
- public NamesFactory(TaskConfig.NamesFactory namesFactory, Task properties) {
- putAll(properties.getAll());
- expandFieldValues(namesFactory, Sets.newHashSet(TaskConfig.KEY_TYPE));
+ /** Use to expand properties specifically for NamesFactories. */
+ public NamesFactory getNamesFactory(NamesFactory namesFactory) {
+ return expand(
+ namesFactory,
+ nf -> namesFactory.config.new NamesFactory(nf),
+ Sets.newHashSet(TaskConfig.KEY_TYPE));
}
}
@@ -153,45 +154,84 @@
protected static final Pattern PATTERN = Pattern.compile("\\$\\{([^}]+)\\}");
/** Expand all properties in the Strings in the object's Fields (except the exclude ones) */
- protected void expandFieldValues(Object object, Set<String> excludedFieldNames) {
- for (Field field : object.getClass().getFields()) {
+ protected <T> T expandFieldValues(T object, Set<String> excludedFieldNames) {
+ // Fake the CopyOnWrite for backwards compatibility.
+ return expand(object, o -> o, 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 occured.
+ */
+ public <T> T expand(T object, Function<T, T> copier, Set<String> excludedFieldNames) {
+ return expand(new CopyOnWrite<>(object, copier), 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 occured.
+ */
+ public <T> T expand(CopyOnWrite<T> cow, Set<String> excludedFieldNames) {
+ for (Field field : cow.getOriginal().getClass().getFields()) {
try {
if (!excludedFieldNames.contains(field.getName())) {
field.setAccessible(true);
- Object o = field.get(object);
+ Object o = field.get(cow.getOriginal());
if (o instanceof String) {
- field.set(object, expandText((String) o));
+ String expanded = expandText((String) o);
+ if (expanded != o) {
+ field.set(cow.getForWrite(), expanded);
+ }
} else if (o instanceof List) {
@SuppressWarnings("unchecked")
List<String> forceCheck = List.class.cast(o);
- expandElements(forceCheck);
+ List<String> expanded = expand(forceCheck);
+ if (expanded != o) {
+ field.set(cow.getForWrite(), expanded);
+ }
}
}
} catch (IllegalAccessException e) {
throw new RuntimeException(e);
}
}
+ return cow.getForRead();
}
- /** Expand all properties in the Strings in the List */
- public void expandElements(List<String> list) {
+ /**
+ * Returns expanded unmodifiable List if property found. Returns same object if no expansions
+ * occured.
+ */
+ public List<String> expand(List<String> list) {
if (list != null) {
- for (ListIterator<String> it = list.listIterator(); it.hasNext(); ) {
- it.set(expandText(it.next()));
+ boolean hasProperty = false;
+ List<String> expandedList = new ArrayList<>(list.size());
+ for (String value : list) {
+ String expanded = expandText(value);
+ hasProperty = hasProperty || value != expanded;
+ expandedList.add(expanded);
}
+ return hasProperty ? Collections.unmodifiableList(expandedList) : list;
}
+ return null;
}
- /** Expand all properties (${property_name} -> property_value) in the given text */
+ /**
+ * Expand all properties (${property_name} -> property_value) in the given text . Returns same
+ * object if no expansions occured.
+ */
public String expandText(String text) {
if (text == null) {
return null;
}
StringBuffer out = new StringBuffer();
Matcher m = PATTERN.matcher(text);
- while (m.find()) {
- m.appendReplacement(out, Matcher.quoteReplacement(getValueForName(m.group(1))));
+ if (!m.find()) {
+ return text;
}
+ do {
+ m.appendReplacement(out, Matcher.quoteReplacement(getValueForName(m.group(1))));
+ } while (m.find());
m.appendTail(out);
return out.toString();
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
index c940ae0..a359764 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -218,10 +218,9 @@
for (String tasksFactoryName : task.subTasksFactories) {
TasksFactory tasksFactory = task.config.getTasksFactory(tasksFactoryName);
if (tasksFactory != null) {
- NamesFactory namesFactory =
- task.config.new NamesFactory(task.config.getNamesFactory(tasksFactory.namesFactory));
+ NamesFactory namesFactory = task.config.getNamesFactory(tasksFactory.namesFactory);
if (namesFactory != null && namesFactory.type != null) {
- new Properties.NamesFactory(namesFactory, getProperties());
+ namesFactory = getProperties().getNamesFactory(namesFactory);
switch (NamesFactoryType.getNamesFactoryType(namesFactory.type)) {
case STATIC:
addStaticTypeTaskDefinitions(tasksFactory, namesFactory);
