Use a COW approach for tasks modified for Properties
Previously a task was modified directly during property expansion. This
made it unsafe to cache tasks output by the preloader and also made it
impossible to update a Node for changes after the first one. Fix this by
copying a task before expanding its properties. Only copy tasks which
actually have properties to be expanded in order to avoid the
performance hit on every task as the copying itself is expensive. Using
a COW approach is much safer and also enables many more optimizations
based on detecting whether a task was altered while eventually
refreshing nodes for each change in a query.
The performance impact of this change seems to be at the noise level.
Change-Id: Idf101dab7990683749c39d82251ece67f127eb9c
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 ae40364..b6d86e1 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
@@ -37,22 +37,33 @@
public class Properties {
public static final Properties EMPTY_PARENT = new Properties();
+ protected final Properties parentProperties;
+ protected final Task origTask;
+ protected final CopyOnWrite<Task> task;
protected Expander expander;
public Properties() {
+ this(null, null);
expander = new Expander();
}
- public Properties(ChangeData changeData, Task definition, Properties parentProperties)
- throws OrmException {
+ public Properties(Task origTask, Properties parentProperties) {
+ this.origTask = origTask;
+ this.parentProperties = parentProperties;
+ task = new CopyOnWrite<>(origTask, t -> origTask.config.new Task(t));
+ }
+
+ public Task getTask(ChangeData changeData) throws OrmException {
expander = new Expander();
expander.putAll(parentProperties.expander.getAll());
- expander.putAll(getInternalProperties(definition, changeData));
- expander.putAll(definition.getAllProperties());
+ expander.putAll(getInternalProperties(origTask, changeData));
+ expander.putAll(origTask.getAllProperties());
- definition.setExpandedProperties(expander.getAll());
-
- expander.expandFieldValues(definition, Collections.emptySet());
+ Map<String, String> exported = expander.expand(origTask.exported);
+ if (exported != origTask.exported) {
+ task.getForWrite().exported = exported;
+ }
+ return expander.expand(task, Collections.emptySet());
}
/** Use to expand properties specifically for NamesFactories. */
@@ -63,11 +74,11 @@
Sets.newHashSet(TaskConfig.KEY_TYPE));
}
- protected static Map<String, String> getInternalProperties(Task definition, ChangeData changeData)
+ protected static Map<String, String> getInternalProperties(Task task, ChangeData changeData)
throws OrmException {
Map<String, String> properties = new HashMap<>();
- properties.put("_name", definition.name);
+ properties.put("_name", task.name);
Change c = changeData.change();
properties.put("_change_number", String.valueOf(c.getId().get()));
@@ -114,6 +125,26 @@
return Collections.unmodifiableMap(valueByName);
}
+ /**
+ * Expand all properties (${property_name} -> property_value) in the given text. Returns same
+ * object if no expansions occured.
+ */
+ public Map<String, String> expand(Map<String, String> map) {
+ if (map != null) {
+ boolean hasProperty = false;
+ Map<String, String> expandedMap = new HashMap<>(map.size());
+ for (Map.Entry<String, String> e : map.entrySet()) {
+ String name = e.getKey();
+ String value = e.getValue();
+ String expanded = getValueForName(name);
+ hasProperty = hasProperty || value != expanded;
+ expandedMap.put(name, expanded);
+ }
+ return hasProperty ? Collections.unmodifiableMap(expandedMap) : map;
+ }
+ return null;
+ }
+
@Override
public String getValueForName(String name) {
if (!expanding.add(name)) {
@@ -157,12 +188,6 @@
// "${_name}" -> group(1) = "_name"
protected static final Pattern PATTERN = Pattern.compile("\\$\\{([^}]+)\\}");
- /** Expand all properties in the Strings in the object's Fields (except the exclude ones) */
- 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.
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 97fefb7..58cb37d 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -123,14 +123,6 @@
return all;
}
- protected void setExpandedProperties(Map<String, String> expanded) {
- properties.clear();
- properties.putAll(expanded);
- for (String property : exported.keySet()) {
- exported.put(property, properties.get(property));
- }
- }
-
public String key() {
// name is needed to differentiate Tasks from the same TasksFactory
return super.key() + SEP + name;
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 16126af..702babb 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -155,13 +155,13 @@
public final Task task;
protected final Properties properties;
- public Node(NodeList parent, Task definition) throws ConfigInvalidException, OrmException {
+ public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException {
this.parent = parent;
- this.task = definition;
+ Preloader.preload(task);
+ properties = new Properties(task, parent.getProperties());
+ this.task = properties.getTask(getChangeData());
this.path.addAll(parent.path);
this.path.add(key());
- Preloader.preload(definition);
- properties = new Properties(getChangeData(), definition, parent.getProperties());
}
public String key() {
