Only reload nodes when needed
If the task hasn't been modified by property expansion and NamesFactory
expansion has not used any change or inherited properties, then the
current Node's subNodes do not need to be reloaded so skip reloading
them. The practical impact of this is that any current operational task
configuration should not be doing much more work after this change than
if Node refreshing had never been implemented in the first place as this
effectively eliminates all the refreshing overhead in those cases.
After this change, the cost of the change properties implementation is
only born by tasks which actually use them. Note: when subNodes use
inherited properties from a parent task, they may still cause all the
subNodes to be reloaded unnecessarilly, this could be further optimized
away at some point if desired.
Change-Id: Ia60006ba53a0055daed3f7c1d4001a74a4233a8a
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 0812004..86fbe72 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
@@ -40,8 +40,10 @@
protected final Task origTask;
protected final CopyOnWrite<Task> task;
protected Expander expander;
+ protected Loader loader;
protected boolean init = true;
protected boolean isTaskRefreshNeeded;
+ protected boolean isSubNodeReloadRequired;
public Properties() {
this(null, null);
@@ -56,7 +58,15 @@
/** Use to expand properties specifically for Tasks. */
public Task getTask(ChangeData changeData) throws OrmException {
- Loader loader = new Loader(changeData);
+ if (loader != null && loader.isNonTaskDefinedPropertyLoaded()) {
+ // To detect NamesFactories dependent on non task defined properties, the checking must be
+ // done after subnodes are fully loaded, which unfortunately happens after getTask() is
+ // called. However, these non task property uses from the last change are still detectable
+ // here before we replace the old Loader with a new one.
+ isSubNodeReloadRequired = true;
+ }
+
+ loader = new Loader(changeData);
expander = new Expander(n -> loader.load(n));
if (isTaskRefreshNeeded || init) {
@@ -75,6 +85,10 @@
return task.getForRead();
}
+ public boolean isSubNodeReloadRequired() {
+ return isSubNodeReloadRequired;
+ }
+
/** Use to expand properties specifically for NamesFactories. */
public NamesFactory getNamesFactory(NamesFactory namesFactory) {
return expander.expand(
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 9de77a4..e98b8fc 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -209,7 +209,7 @@
this.task = properties.getTask(getChangeData());
- if (nodes != null) {
+ if (nodes != null && properties.isSubNodeReloadRequired()) {
cachedNodeByTask.clear();
nodes.stream().filter(n -> n != null).forEach(n -> cachedNodeByTask.put(n.task.key(), n));
names.clear();
