Cache applicable definitionLists for ChangeNodes Take advantage of ChangeNodes whose subNodes do not require a reload to cache applicable, filtered lists, per branch. For non Change Nodes, the subNodes lists, including applicable, filtered subNode Lists are already cached per branch. However for ChangeNodes these caches are always empty since ChangeNodes are not reused (due to their name changing). Much of the benefits of this missed caching can be achieved for ChangeNodes by at least caching definitionLists instead of NodeLists. This is already being done for the full definitionList for ChangeNodes, so do it also for the applicable, filtered definitionLists, per branch. In a sample walking ancestors use case, this definitionList caching for over 33K ChangeNodes results applicable nodelists being evaluated less than 300 times! The performance gain in the case of a task.config which walks all dependencies for a change run with status:open --no-limit --task--applicable is very significant, around three times as fast: Before this change: 15m14s 15m46s 17m37s 15m24s 15m11s After this change: 4m47s 4m56s 4m22s 5m5s 4m27s Change-Id: I6a7ef48b028e901179a0767734aec709bec9c7cd
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java b/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java index ff9ff3b..a7f0f72 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/HitHashMap.java
@@ -14,7 +14,11 @@ package com.googlesource.gerrit.plugins.task; +import static java.util.stream.Collectors.toList; + import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.function.BiFunction; import java.util.function.Function; @@ -22,12 +26,21 @@ public static class Statistics { public long hits; public int size; + public List<Object> elements; } public static final long serialVersionUID = 1; protected Statistics statistics; + public HitHashMap() {} + + public HitHashMap(boolean initStatistics) { + if (initStatistics) { + initStatistics(); + } + } + @Override public V get(Object key) { V v = super.get(key); @@ -59,6 +72,28 @@ } @Override + public V put(K key, V value) { + if (statistics != null && value instanceof TracksStatistics) { + ((TracksStatistics) value).ensureStatistics(); + } + return super.put(key, value); + } + + @Override + public void putAll(Map<? extends K, ? extends V> m) { + m.entrySet().stream().forEach(e -> put(e.getKey(), e.getValue())); + } + + @Override + public V putIfAbsent(K key, V value) { + if (!containsKey(key)) { + put(key, value); + return null; + } + return get(key); + } + + @Override public V computeIfPresent( K key, BiFunction<? super K, ? super V, ? extends V> remappingFunction) { throw new UnsupportedOperationException(); // Todo if needed @@ -75,13 +110,43 @@ } @Override + public V replace(K key, V value) { + throw new UnsupportedOperationException(); // Todo if needed + } + + @Override + public boolean replace(K key, V oldValue, V newValue) { + throw new UnsupportedOperationException(); // Todo if needed + } + + @Override + public void replaceAll(BiFunction<? super K, ? super V, ? extends V> function) { + throw new UnsupportedOperationException(); // Todo if needed + } + + @Override public void initStatistics() { statistics = new Statistics(); } @Override + public void ensureStatistics() { + if (statistics == null) { + initStatistics(); + } + } + + @Override public Object getStatistics() { statistics.size = size(); + List<Object> elementStatistics = + values().stream() + .filter(e -> e instanceof TracksStatistics) + .map(e -> ((TracksStatistics) e).getStatistics()) + .collect(toList()); + if (!elementStatistics.isEmpty()) { + statistics.elements = elementStatistics; + } return statistics; } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/StatisticsMap.java b/src/main/java/com/googlesource/gerrit/plugins/task/StatisticsMap.java index aeeb434..4ae8857 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/StatisticsMap.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/StatisticsMap.java
@@ -16,8 +16,4 @@ import java.util.Map; -public interface StatisticsMap<K, V> extends Map<K, V> { - void initStatistics(); - - Object getStatistics(); -} +public interface StatisticsMap<K, V> extends Map<K, V>, TracksStatistics {}
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 6349156..64b333f 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -69,6 +69,7 @@ public static class Statistics { public Object definitionsPerSubSectionCache; + public Object definitionsByBranchBySubSectionCache; } protected static final String TASK_DIR = "task"; @@ -82,6 +83,8 @@ protected final Provider<ChangeQueryProcessor> changeQueryProcessorProvider; protected final StatisticsMap<SubSectionKey, List<Task>> definitionsBySubSection = new HitHashMapOfCollection<>(); + protected final StatisticsMap<SubSectionKey, Map<BranchNameKey, List<Task>>> + definitionsByBranchBySubSection = new HitHashMap<>(); protected ChangeData changeData; protected Statistics statistics; @@ -456,6 +459,9 @@ protected MatchCache matchCache; protected PredicateCache pcache; protected BranchNameKey branch = getChangeData().change().getDest(); + protected SubSectionKey subSection = task.key.subSection(); + protected Map<BranchNameKey, List<Task>> definitionsByBranch = + definitionsByBranchBySubSection.get(subSection); public ApplicableNodeFilter(MatchCache matchCache) throws ConfigInvalidException, IOException, StorageException { @@ -470,18 +476,42 @@ return refresh(nodes); } } - + if (definitionsByBranch != null) { + List<Task> branchDefinitions = definitionsByBranch.get(branch); + if (branchDefinitions != null) { + return new SubNodeFactory().createFromPreloaded(branchDefinitions); + } + } List<Node> nodes = Node.this.getSubNodes(); + if (isChange() + && definitionsByBranch == null + && definitionsByBranchBySubSection.containsKey(subSection)) { + hasUnfilterableSubNodes = true; + } + if (!hasUnfilterableSubNodes && !nodes.isEmpty()) { Optional<List<Node>> filterable = getOptionalApplicableForBranch(nodes); if (filterable.isPresent()) { - if (nodesByBranch == null) { - nodesByBranch = new HitHashMapOfCollection<>(statistics != null); + if (!isChange()) { + if (nodesByBranch == null) { + nodesByBranch = new HitHashMapOfCollection<>(statistics != null); + } + nodesByBranch.put(branch, filterable.get()); + } else { + if (definitionsByBranch == null) { + definitionsByBranch = new HitHashMap<>(statistics != null); + definitionsByBranchBySubSection.put(subSection, definitionsByBranch); + } + definitionsByBranch.put( + branch, + filterable.get().stream().map(node -> node.getDefinition()).collect(toList())); } - nodesByBranch.put(branch, filterable.get()); return filterable.get(); } hasUnfilterableSubNodes = true; + if (isChange()) { + definitionsByBranchBySubSection.put(subSection, null); + } } return nodes; } @@ -556,11 +586,14 @@ public void initStatistics() { statistics = new Statistics(); definitionsBySubSection.initStatistics(); + definitionsByBranchBySubSection.initStatistics(); } public Statistics getStatistics() { if (statistics != null) { statistics.definitionsPerSubSectionCache = definitionsBySubSection.getStatistics(); + statistics.definitionsByBranchBySubSectionCache = + definitionsByBranchBySubSection.getStatistics(); } return statistics; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TracksStatistics.java b/src/main/java/com/googlesource/gerrit/plugins/task/TracksStatistics.java new file mode 100644 index 0000000..d7d0651 --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TracksStatistics.java
@@ -0,0 +1,23 @@ +// Copyright (C) 2022 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.task; + +public interface TracksStatistics { + void initStatistics(); + + void ensureStatistics(); + + Object getStatistics(); +}