commit 16d688ea53fa9a1c4edcc1fa87931552d41c1903
author Martin Fick <martin.fick@linaro.org> Tue Jul 26 21:20:46 2022 -0600
committer Martin Fick <martin.fick@linaro.org> Wed Sep 07 15:40:30 2022 -0600
tree 0fb54dffed6f956ae621f5a505f8132b20df6e0f
parent fd700764a05ebb1bfb19fe2038d96a3478365c8c

Cache Task definition lists for ChangeNodes

ChangeNodes do not benefit from caching their subNode lists even when
they are not going to need to be reloaded because ChangeNodes themselves
aren't cached or reused due to their name change. However if the
subNodes don't need to be reloaded, the subtask lists will be the same
for other ChangeNodes created by the same TasksFactory. In this case,
cache the Task definitions Lists by TasksFactory and reuse them instead
of node lists. ChangeNodes can still benefit from not repeating the
related work that was done to create the definitions: expanding tasks,
fetching definitions, creating definitions from factories...

A walking ancestors use case benefits tremendously from this reuse since
a top level wlaking task, and thus all the ChangeNodes do not need their
subtasks reloaded, and there are over 1K of these subnodes, and
potentially around 100K ChangeNodes when walking ancestors!

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 very significant, around 2.5 times faster:

Before this change: 23m4s, 24m48s, 26m38s, 24m18s, 25m2s
After this change: 9m29s, 9m25s, 9m28s, 9m33s, 11m11s

Change-Id: I2ef200e629627a76936d78a6aec8fe460cd46501

src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java

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 e246af8..8992a91 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -14,6 +14,8 @@
 
 package com.googlesource.gerrit.plugins.task;
 
+import static java.util.stream.Collectors.toList;
+
 import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.index.query.QueryParseException;
 import com.google.gerrit.reviewdb.client.Account;
@@ -73,6 +75,7 @@
   protected final NodeList root = new NodeList();
   protected final Provider<ChangeQueryBuilder> changeQueryBuilderProvider;
   protected final Provider<ChangeQueryProcessor> changeQueryProcessorProvider;
+  protected final Map<SubSectionKey, List<Task>> definitionsBySubSection = new HashMap<>();
 
   protected ChangeData changeData;
 
@@ -230,13 +233,18 @@
       }
       List<Node> nodes = loadSubNodes();
       if (!properties.isSubNodeReloadRequired()) {
-        return cachedNodes = nodes;
+        if (!isChange()) {
+          return cachedNodes = nodes;
+        }
+        definitionsBySubSection.computeIfAbsent(
+            task.key().subSection(), k -> nodes.stream().map(n -> n.task).collect(toList()));
+      } else {
+        hasUnfilterableSubNodes = true;
+        cachedNodeByTask.clear();
+        nodes.stream()
+            .filter(n -> !(n instanceof Invalid) && !n.isChange())
+            .forEach(n -> cachedNodeByTask.put(n.task.key(), n));
       }
-      hasUnfilterableSubNodes = true;
-      cachedNodeByTask.clear();
-      nodes.stream()
-          .filter(n -> !(n instanceof Invalid) && !n.isChange())
-          .forEach(n -> cachedNodeByTask.put(n.task.key(), n));
       return nodes;
     }
 
@@ -250,6 +258,10 @@
 
     @Override
     protected List<Node> loadSubNodes() throws ConfigInvalidException, IOException, OrmException {
+      List<Task> cachedDefinitions = definitionsBySubSection.get(task.key().subSection());
+      if (cachedDefinitions != null) {
+        return new SubNodeFactory().createFromPreloaded(cachedDefinitions);
+      }
       List<Node> nodes = new SubNodeAdder().getSubNodes();
       properties.expansionComplete();
       return nodes;