Add a TaskTree ApplicableNodeFilter
The ApplicableNodeFilter is able to determine whether a node can cache
the applicability of some of its subnodes by Branch. If so, it will
cache this applicability for each Branch which it encounters, and reuse
the results when it encounters the same branch again. This potentially
reduces the rescanning of large numbers of subnodes for each Change down
to rescanning those nodes only for each Branch.
Due to the nature of a global server task config, task applicability is
often determined by a Change's project and branch, so it would not be
unusual for this per Branch caching to benefit large task configs. It is
relevant to note that since ChangeNodes are not cached, this change does
not improve walking dependencies. Further more sophisticated
improvements are needed to noticeably improve the "walking" use case.
In a real world use case, there is a root task with over 1K subtasks and
only 1 or 2 of those subtasks is almost ever applicable to a Change. The
applicability in these cases is almost exclusively determined by
'destination' and `destination like` queries which are based on the
Change's destination. In this case, when the PW root task is being
evaluated on a Change destined for an already seen Branch, instead of
having to evaluate over 1K subnodes against the Change to see if they
are applicable, the 1 or 2 applicable subnodes can be returned from the
cache using a simple Branch lookup in a Map.
With this change, there is a significant performance increase of the
following ssh non change walking query:
status:open --task--applicable --format json --no-limit
Before this change: 1m37s, 1m42s, 1m37s
After this change: 1m11s, 1m10s, 1m14s
Change-Id: I4c3ff9ceb990386e0f0d11fbd55cdc825c677e4c
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
index b3f269f..690b035 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskAttributeFactory.java
@@ -260,7 +260,8 @@
protected List<TaskAttribute> getSubTasks()
throws ConfigInvalidException, IOException, OrmException {
List<TaskAttribute> subTasks = new ArrayList<>();
- for (Node subNode : node.getSubNodes()) {
+ for (Node subNode :
+ options.onlyApplicable ? node.getSubNodes(matchCache) : node.getSubNodes()) {
if (subNode == null) {
subTasks.add(invalid());
} else {
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 9775643..06ce146 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -116,24 +116,13 @@
if (cachedNodes == null) {
return loadSubNodes();
}
- refreshSubNodes();
- return cachedNodes;
+ return refresh(cachedNodes);
}
protected List<Node> loadSubNodes() throws ConfigInvalidException, IOException, OrmException {
return cachedNodes = new SubNodeAdder().getSubNodes();
}
- public void refreshSubNodes() throws ConfigInvalidException, OrmException {
- if (cachedNodes != null) {
- for (Node node : cachedNodes) {
- if (node != null) {
- node.refreshTask();
- }
- }
- }
- }
-
public ChangeData getChangeData() {
return parent == null ? TaskTree.this.changeData : parent.getChangeData();
}
@@ -205,6 +194,8 @@
protected final Properties properties;
protected final TaskKey taskKey;
+ protected Map<Branch.NameKey, List<Node>> nodesByBranch;
+ protected boolean hasUnfilterableSubNodes = false;
public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException {
this.parent = parent;
@@ -217,6 +208,14 @@
return String.valueOf(getChangeData().getId().get()) + TaskConfig.SEP + taskKey;
}
+ public List<Node> getSubNodes(MatchCache matchCache)
+ throws ConfigInvalidException, IOException, OrmException {
+ if (hasUnfilterableSubNodes) {
+ return getSubNodes();
+ }
+ return new ApplicableNodeFilter(matchCache).getSubNodes();
+ }
+
@Override
protected List<Node> loadSubNodes() throws ConfigInvalidException, IOException, OrmException {
List<Node> nodes = new SubNodeAdder().getSubNodes();
@@ -224,6 +223,7 @@
if (!properties.isSubNodeReloadRequired()) {
cachedNodes = nodes;
} else {
+ hasUnfilterableSubNodes = true;
cachedNodeByTask.clear();
nodes.stream()
.filter(n -> n != null && !n.isChange())
@@ -383,6 +383,79 @@
FileKey.create(resolveUserBranch(external.user), resolveTaskFileName(external.file)));
}
}
+
+ public class ApplicableNodeFilter {
+ protected MatchCache matchCache;
+ protected PredicateCache pcache;
+ protected Branch.NameKey branch = getChangeData().change().getDest();
+
+ public ApplicableNodeFilter(MatchCache matchCache)
+ throws ConfigInvalidException, IOException, OrmException {
+ this.matchCache = matchCache;
+ this.pcache = matchCache.predicateCache;
+ }
+
+ public List<Node> getSubNodes() throws ConfigInvalidException, IOException, OrmException {
+ if (nodesByBranch != null) {
+ List<Node> nodes = nodesByBranch.get(branch);
+ if (nodes != null) {
+ return refresh(nodes);
+ }
+ }
+
+ List<Node> nodes = Node.this.getSubNodes();
+ if (!hasUnfilterableSubNodes && !nodes.isEmpty()) {
+ Optional<List<Node>> filterable = getOptionalApplicableForBranch(nodes);
+ if (filterable.isPresent()) {
+ if (nodesByBranch == null) {
+ nodesByBranch = new HashMap<>();
+ }
+ nodesByBranch.put(branch, filterable.get());
+ return filterable.get();
+ }
+ hasUnfilterableSubNodes = true;
+ }
+ return nodes;
+ }
+
+ protected Optional<List<Node>> getOptionalApplicableForBranch(List<Node> nodes)
+ throws ConfigInvalidException, IOException, OrmException {
+ int filterable = 0;
+ List<Node> applicableNodes = new ArrayList<>();
+ for (Node node : nodes) {
+ if (node != null && isApplicableCacheableByBranch(node)) {
+ filterable++;
+ try {
+ if (!matchCache.match(node.task.applicable)) {
+ // Correctness will not be affected if more nodes are added than necessary
+ // (i.e. if isApplicableCacheableByBranch() does not realize a Node is cacheable
+ // based on its Branch), but it is incorrect to filter out a Node now that could
+ // later be applicable when a property, other than its Change's destination, is
+ // altered.
+ continue;
+ }
+ } catch (QueryParseException e) {
+ }
+ }
+ applicableNodes.add(node);
+ }
+ // Simple heuristic to determine whether storing the filtered nodes is worth it. There
+ // is minor evidence to suggest that storing a large list actually hurts performance.
+ return (filterable > nodes.size() / 2) ? Optional.of(applicableNodes) : Optional.empty();
+ }
+
+ protected boolean isApplicableCacheableByBranch(Node node) {
+ String applicable = node.task.applicable;
+ if (node.properties.isApplicableRefreshRequired()) {
+ return false;
+ }
+ try {
+ return pcache.isCacheableByBranch(applicable);
+ } catch (QueryParseException e) {
+ return false;
+ }
+ }
+ }
}
protected String resolveTaskFileName(String file) throws ConfigInvalidException {
@@ -407,4 +480,14 @@
}
return new Branch.NameKey(allUsers.get(), RefNames.refsUsers(acct.getId()));
}
+
+ protected static List<Node> refresh(List<Node> nodes)
+ throws ConfigInvalidException, OrmException {
+ for (Node node : nodes) {
+ if (node != null) {
+ node.refreshTask();
+ }
+ }
+ return nodes;
+ }
}
diff --git a/src/main/resources/Documentation/test/task_states.md b/src/main/resources/Documentation/test/task_states.md
index fede191..71ee0ec 100644
--- a/src/main/resources/Documentation/test/task_states.md
+++ b/src/main/resources/Documentation/test/task_states.md
@@ -1183,6 +1183,99 @@
]
}
+[root "Root applicable Property"]
+ subtask = Subtask applicable Property
+ subtasks-factory = tasks-factory branch NOT applicable Property
+
+[tasks-factory "tasks-factory branch NOT applicable Property"]
+ names-factory = names-factory branch NOT applicable Property
+ applicable = branch:dev
+ fail = True
+
+[names-factory "names-factory branch NOT applicable Property"]
+ type = static
+ name = NOT Applicable 1
+ name = NOT Applicable 2
+ name = NOT Applicable 3
+
+[task "Subtask applicable Property"]
+ applicable = change:${_change_number}
+ fail = True
+
+{
+ "applicable" : true,
+ "hasPass" : false,
+ "name" : "Root applicable Property",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Subtask applicable Property",
+ "status" : "FAIL"
+ }, # Only Test Suite: all
+ { # Only Test Suite: all
+ "applicable" : false, # Only Test Suite: all
+ "hasPass" : true, # Only Test Suite: all
+ "name" : "NOT Applicable 1", # Only Test Suite: all
+ "status" : "FAIL" # Only Test Suite: all
+ }, # Only Test Suite: all
+ { # Only Test Suite: all
+ "applicable" : false, # Only Test Suite: all
+ "hasPass" : true, # Only Test Suite: all
+ "name" : "NOT Applicable 2", # Only Test Suite: all
+ "status" : "FAIL" # Only Test Suite: all
+ }, # Only Test Suite: all
+ { # Only Test Suite: all
+ "applicable" : false, # Only Test Suite: all
+ "hasPass" : true, # Only Test Suite: all
+ "name" : "NOT Applicable 3", # Only Test Suite: all
+ "status" : "FAIL" # Only Test Suite: all
+ }
+ ]
+}
+
+[root "Root branch applicable Property"]
+ subtasks-factory = tasks-factory branch applicable Property
+
+[tasks-factory "tasks-factory branch applicable Property"]
+ names-factory = names-factory branch applicable Property
+ applicable = branch:master
+ fail = True
+
+[names-factory "names-factory branch applicable Property"]
+ type = static
+ name = Applicable 1
+ name = Applicable 2
+ name = Applicable 3
+
+{
+ "applicable" : true,
+ "hasPass" : false,
+ "name" : "Root branch applicable Property",
+ "status" : "WAITING",
+ "subTasks" : [
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Applicable 1",
+ "status" : "FAIL"
+ },
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Applicable 2",
+ "status" : "FAIL"
+ },
+ {
+ "applicable" : true,
+ "hasPass" : true,
+ "name" : "Applicable 3",
+ "status" : "FAIL"
+ }
+ ]
+}
+
[root "Root Properties tasks-factory STATIC"]
subtasks-factory = tasks-factory STATIC Properties
