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
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;