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