Refresh TaskTree.Nodes when getting them With the introduction of Change properties, Node attributes and their subnodes may depend on the specific change properties they were created for. In order to correct current over-caching behavior which results in erroneous Node contents when more than one change is evaluated, refresh a Node's attributes that may be dependent on that change anytime the Node is returned by a parent Node. Throwing away subnodes from a Node can be expensive since it results in all the Nodes' descendants also being discarded. To reduce this cost, this change reuses subnodes with the same key after a refresh. Also enhance the test framework to have a test suite which evaluates a second change's output to ensure that the output is updated appropriately beyond the first change. Despite much re-use, this change still results in an approximate 5x performance degradation on our Gerrit 2.7 fork. Change-Id: Ib598b4c3a8e4fdbef0f41027cb1e6c7be5abe0d8
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 7d3e075..9de77a4 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -39,9 +39,13 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -96,12 +100,14 @@ public List<Node> getRootNodes(ChangeData changeData) throws ConfigInvalidException, IOException, OrmException { this.changeData = changeData; + root.path = Collections.emptyList(); return root.getSubNodes(); } protected class NodeList { protected NodeList parent = null; - protected LinkedList<String> path = new LinkedList<>(); + protected Collection<String> path; + protected Map<String, Node> cachedNodeByTask = new HashMap<>(); protected List<Node> nodes; protected Set<String> names = new HashSet<>(); @@ -109,23 +115,32 @@ addPreloaded(taskFactory.getRootConfig().getPreloadedRootTasks()); } - protected void addPreloaded(List<Task> defs) { + protected void addPreloaded(List<Task> defs) throws ConfigInvalidException, OrmException { for (Task def : defs) { addPreloaded(def); } } - protected void addPreloaded(Task def) { + protected void addPreloaded(Task def) throws ConfigInvalidException, OrmException { addPreloaded(def, (parent, definition) -> new Node(parent, definition)); } - protected void addPreloaded(Task def, NodeFactory nodeFactory) { + protected void addPreloaded(Task def, NodeFactory nodeFactory) + throws ConfigInvalidException, OrmException { if (def != null) { try { - Node node = nodeFactory.create(this, def); + Node node = cachedNodeByTask.get(def.key()); + boolean isRefreshNeeded = node != null; + if (node == null) { + node = nodeFactory.create(this, def); + } + if (!path.contains(node.key()) && names.add(def.name)) { // path check above detects looping definitions // names check above detects duplicate subtasks + if (isRefreshNeeded) { + node.refreshTask(); + } nodes.add(node); return; } @@ -143,10 +158,22 @@ if (nodes == null) { nodes = new ArrayList<>(); addSubNodes(); + } else { + refreshSubNodes(); } return nodes; } + public void refreshSubNodes() throws ConfigInvalidException, OrmException { + if (nodes != null) { + for (Node node : nodes) { + if (node != null) { + node.refreshTask(); + } + } + } + } + public ChangeData getChangeData() { return parent == null ? TaskTree.this.changeData : parent.getChangeData(); } @@ -157,19 +184,37 @@ } public class Node extends NodeList { - public final Task task; + public Task task; + protected final Properties properties; + protected final String taskKey; public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException { this.parent = parent; + taskKey = task.key(); properties = new Properties(task, parent.getProperties()); - this.task = properties.getTask(getChangeData()); - this.path.addAll(parent.path); - this.path.add(key()); + refreshTask(); } public String key() { - return String.valueOf(getChangeData().getId().get()) + TaskConfig.SEP + task.key(); + return String.valueOf(getChangeData().getId().get()) + TaskConfig.SEP + taskKey; + } + + /* The task needs to be refreshed before a node is used, however + subNode refreshing can wait until they are fetched since they may + not be needed. */ + public void refreshTask() throws ConfigInvalidException, OrmException { + this.path = new LinkedList<>(parent.path); + this.path.add(key()); + + this.task = properties.getTask(getChangeData()); + + if (nodes != null) { + cachedNodeByTask.clear(); + nodes.stream().filter(n -> n != null).forEach(n -> cachedNodeByTask.put(n.task.key(), n)); + names.clear(); + nodes = null; + } } @Override @@ -180,7 +225,7 @@ addSubTasksExternals(); } - protected void addSubTasks() { + protected void addSubTasks() throws ConfigInvalidException, OrmException { for (String expression : task.subTasks) { try { Optional<Task> def = task.config.getPreloadedOptionalTask(new TaskExpression(expression)); @@ -193,7 +238,7 @@ } } - protected void addSubTasksFiles() { + protected void addSubTasksFiles() throws ConfigInvalidException, OrmException { for (String file : task.subTasksFiles) { try { addPreloaded(getPreloadedTasks(task.config.getBranch(), file)); @@ -203,7 +248,7 @@ } } - protected void addSubTasksExternals() throws OrmException { + protected void addSubTasksExternals() throws ConfigInvalidException, OrmException { for (String external : task.subTasksExternals) { try { External ext = task.config.getExternal(external); @@ -240,14 +285,14 @@ } protected void addStaticTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) - throws ConfigInvalidException { + throws ConfigInvalidException, OrmException { for (String name : namesFactory.names) { addPreloaded(preload(task.config.new Task(tasksFactory, name))); } } protected void addChangeTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) - throws ConfigInvalidException { + throws ConfigInvalidException, OrmException { try { if (namesFactory.changes != null) { List<ChangeData> changeDataList =
diff --git a/test/check_task_statuses.sh b/test/check_task_statuses.sh index ae63924..527442d 100755 --- a/test/check_task_statuses.sh +++ b/test/check_task_statuses.sh
@@ -37,9 +37,9 @@ result "$name" "$(diff <(echo "$expected") <(echo "$actual"))" } -result_root() { # group root expected_file actual_file +result_root() { # group root expected_file actual_json local name="$1 - $(echo "$2" | sed -es'/Root //')" - result_out "$name" "$(get_root "$2" < "$3")" "$(get_root "$2" < "$4")" + result_out "$name" "$(get_root "$2" < "$3")" "$(echo "$4" | get_root "$2")" } # -------- Git Config @@ -199,7 +199,13 @@ print json.dumps(root, indent=3, separators=(',', ' : '), sort_keys=True)" } -example() { # example_num +get_plugins() { # < change_json > plugins_json + python -c "import sys, json; \ + plugins={}; plugins['plugins']=json.loads(sys.stdin.read())['plugins']; \ + print json.dumps(plugins, indent=3, separators=(',', ' : '), sort_keys=True)" +} + +example() { # example_num > text_for_example_num echo "$DOC_STATES" | awk '/```/{Q++;E=(Q+1)/2};E=='"$1" | grep -v '```' | replace_user } @@ -243,7 +249,7 @@ local repo=$1 remote=$2 ref=$3 change_id=$4 msg="Test change" ( q cd "$repo" - date > file + uuidgen > file q git add . [ -n "$change_id" ] && msg=$(commit_message "$msg" "$change_id") q git commit -m "$msg" @@ -251,28 +257,34 @@ ) } -query_plugins() { # query - gssh query "$@" --format json | head -1 | python -c "import sys, json; \ - plugins={}; plugins['plugins']=json.loads(sys.stdin.read())['plugins']; \ - print json.dumps(plugins, indent=3, separators=(',', ' : '), sort_keys=True)" -} +query() { gssh query "$@" --format json ; } # query > json lines -test_tasks() { # name expected_file task_args... - local name=$1 expected=$2 ; shift 2 - local output=$STATUSES.$name out root +# N < json lines > changeN_json +change_plugins() { awk "NR==$1" | get_plugins | json_pp ; } - query_plugins "$@" > "$output" +results_suite() { # name expected_file plugins_json + local name=$1 expected=$2 actual=$3 + local out root + echo "$ROOTS" | while read root ; do - result_root "$name" "$root" "$expected" "$output" + result_root "$name" "$root" "$expected" "$actual" done - out=$(diff "$expected" "$output" | head -15) + out=$(diff "$expected" <(echo "$actual") | head -15) [ -z "$out" ] result "$name - Full Test Suite" "$out" } +test_2generated() { # name task_args... + local name=$1 ; shift + local out=$(query "$@") + results_suite "$name" "$EXPECTED.$name" "$(echo "$out" | change_plugins 1)" + results_suite "$name 2nd change" "$EXPECTED.$name"2 "$(echo "$out" | change_plugins 2)" +} + test_generated() { # name task_args... local name=$1 ; shift - test_tasks "$name" "$EXPECTED.$name" "$@" + query "$@" | change_plugins 1 > "$ACTUAL.$name" + results_suite "$name" "$EXPECTED.$name" "$( < "$ACTUAL.$name")" } test_file() { # name task_args... @@ -297,7 +309,7 @@ DOC_PREVIEW=$DOCS/preview.md EXPECTED=$OUT/expected -STATUSES=$OUT/statuses +ACTUAL=$OUT/actual ROOT_CFG=$ALL/task.config COMMON_CFG=$ALL_TASKS/common.config @@ -342,9 +354,12 @@ q_setup update_repo "$USERS" "$REMOTE_USERS" "$REF_USERS" change3_id=$(gen_change_id) +change4_id=$(gen_change_id) +change4_number=$(create_repo_change "$OUT/$PROJECT" "$REMOTE_TEST" "$BRANCH" "$change4_id") change3_number=$(create_repo_change "$OUT/$PROJECT" "$REMOTE_TEST" "$BRANCH" "$change3_id") -all_pjson=$(example 2 | testdoc_2_pjson | \ +ex2_pjson=$(example 2 | testdoc_2_pjson) +all_pjson=$(echo "$ex2_pjson" | \ replace_change_properties \ "" \ "$change3_number" \ @@ -354,10 +369,23 @@ "NEW" \ "") +all2_pjson=$(echo "$ex2_pjson" | \ + replace_change_properties \ + "" \ + "$change4_number" \ + "$change4_id" \ + "$PROJECT" \ + "refs\/heads\/$BRANCH" \ + "NEW" \ + "") + no_all_json=$(echo "$all_pjson" | remove_suite all) +no_all2_json=$(echo "$all2_pjson" | remove_suite all) echo "$no_all_json" | strip_non_applicable | \ grep -v "\"applicable\" :" > "$EXPECTED".applicable +echo "$no_all2_json" | strip_non_applicable | \ + grep -v "\"applicable\" :" > "$EXPECTED".applicable2 echo "$all_pjson" | remove_not_suite all | ensure json_pp > "$EXPECTED".all @@ -376,8 +404,8 @@ RESULT=0 -query="change:$change3_number status:open" -test_generated applicable --task--applicable "$query" +query="(change:$change3_number OR change:$change4_number) status:open" +test_2generated applicable --task--applicable "$query" test_generated all --task--all "$query" test_generated invalid --task--invalid "$query"