Cache preloaded tasks Since task preloading is often used as a config sharing mechanism, it is not uncommon for many tasks to preload the same task. Additionally many tasks preload tasks which also preload other shared tasks, in our current production site, there are as many as 5 levels of preloading. Cache all preloaded tasks to avoid having to reload each level if the level has already been preloaded by another task (which is very common). Change-Id: I1ecc7d139141fdaadcb7f9d25ee61757314de382
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java index d16c5f6..6af576c 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/Preloader.java
@@ -25,13 +25,33 @@ /** Use to pre-load a task definition with values from its preload-task definition. */ public class Preloader { - public static Task preload(Task definition) throws ConfigInvalidException { + protected final TaskConfig config; + protected final Map<String, Optional<Task>> optionalTaskByName = new HashMap<>(); + + public Preloader(TaskConfig config) { + this.config = config; + } + + public Optional<Task> preloadOptional(TaskExpression expression) throws ConfigInvalidException { + Optional<Task> task = optionalTaskByName.get(expression.getKey()); + if (task == null) { + task = loadOptional(expression); + optionalTaskByName.put(expression.getKey(), task); + } + return task; + } + + protected Optional<Task> loadOptional(TaskExpression expression) throws ConfigInvalidException { + Optional<Task> definition = config.getOptionalTask(expression); + return definition.isPresent() ? Optional.of(preload(definition.get())) : definition; + } + + public Task preload(Task definition) throws ConfigInvalidException { String expression = definition.preloadTask; if (expression != null) { - Optional<Task> preloadFrom = - definition.config.getOptionalTask(new TaskExpression(expression)); + Optional<Task> preloadFrom = preloadOptional(new TaskExpression(expression)); if (preloadFrom.isPresent()) { - return preloadFrom(definition, preload(preloadFrom.get())); + return preloadFrom(definition, preloadFrom.get()); } } return definition;
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java index e1a079b..37f18b5 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskConfig.java
@@ -208,18 +208,33 @@ public boolean isVisible; public boolean isTrusted; + protected final Preloader preloader; + public TaskConfig(Branch.NameKey branch, String fileName, boolean isVisible, boolean isTrusted) { super(branch, fileName); this.isVisible = isVisible; this.isTrusted = isTrusted; + preloader = new Preloader(this); } - public List<Task> getRootTasks() { - return getTasks(SECTION_ROOT); + public List<Task> getPreloadedRootTasks() { + return getPreloadedTasks(SECTION_ROOT); } - public List<Task> getTasks() { - return getTasks(SECTION_TASK); + public List<Task> getPreloadedTasks() { + return getPreloadedTasks(SECTION_TASK); + } + + protected List<Task> getPreloadedTasks(String type) { + List<Task> preloaded = new ArrayList<>(); + for (Task task : getTasks(type)) { + try { + preloaded.add(preloader.preload(task)); + } catch (ConfigInvalidException e) { + preloaded.add(null); + } + } + return preloaded; } protected List<Task> getTasks(String type) { @@ -241,13 +256,19 @@ } /** - * Get a Task for this TaskExpression. + * Get a preloaded Task for this TaskExpression. * * @param TaskExpression * @return Optional<Task> which is empty if the expression is optional and no tasks are resolved * @throws ConfigInvalidException if the expression requires a task and no tasks are resolved */ - public Optional<Task> getOptionalTask(TaskExpression expression) throws ConfigInvalidException { + public Optional<Task> getPreloadedOptionalTask(TaskExpression expression) + throws ConfigInvalidException { + return preloader.preloadOptional(expression); + } + + protected Optional<Task> getOptionalTask(TaskExpression expression) + throws ConfigInvalidException { try { for (String name : expression) { Optional<Task> task = getOptionalTask(name);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java index ee55937..036071b 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskExpression.java
@@ -44,6 +44,10 @@ this.expression = expression; } + public String getKey() { + return expression; + } + @Override public Iterator<String> iterator() { return new Iterator<String>() {
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 0b54745..7d3e075 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java +++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -106,20 +106,20 @@ protected Set<String> names = new HashSet<>(); protected void addSubNodes() throws ConfigInvalidException, IOException, OrmException { - addSubDefinitions(taskFactory.getRootConfig().getRootTasks()); + addPreloaded(taskFactory.getRootConfig().getPreloadedRootTasks()); } - protected void addSubDefinitions(List<Task> defs) { + protected void addPreloaded(List<Task> defs) { for (Task def : defs) { - addSubDefinition(def); + addPreloaded(def); } } - protected void addSubDefinition(Task def) { - addSubDefinition(def, (parent, definition) -> new Node(parent, definition)); + protected void addPreloaded(Task def) { + addPreloaded(def, (parent, definition) -> new Node(parent, definition)); } - protected void addSubDefinition(Task def, NodeFactory nodeFactory) { + protected void addPreloaded(Task def, NodeFactory nodeFactory) { if (def != null) { try { Node node = nodeFactory.create(this, def); @@ -162,7 +162,7 @@ public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException { this.parent = parent; - properties = new Properties(Preloader.preload(task), parent.getProperties()); + properties = new Properties(task, parent.getProperties()); this.task = properties.getTask(getChangeData()); this.path.addAll(parent.path); this.path.add(key()); @@ -173,19 +173,19 @@ } @Override - protected void addSubNodes() throws OrmException { - addSubTaskDefinitions(); - addSubTasksFactoryDefinitions(); - addSubFileDefinitions(); - addExternalDefinitions(); + protected void addSubNodes() throws ConfigInvalidException, OrmException { + addSubTasks(); + addSubTasksFactoryTasks(); + addSubTasksFiles(); + addSubTasksExternals(); } - protected void addSubTaskDefinitions() { + protected void addSubTasks() { for (String expression : task.subTasks) { try { - Optional<Task> def = task.config.getOptionalTask(new TaskExpression(expression)); + Optional<Task> def = task.config.getPreloadedOptionalTask(new TaskExpression(expression)); if (def.isPresent()) { - addSubDefinition(def.get()); + addPreloaded(def.get()); } } catch (ConfigInvalidException e) { addInvalidNode(); @@ -193,24 +193,24 @@ } } - protected void addSubFileDefinitions() { + protected void addSubTasksFiles() { for (String file : task.subTasksFiles) { try { - addSubDefinitions(getTaskDefinitions(task.config.getBranch(), file)); + addPreloaded(getPreloadedTasks(task.config.getBranch(), file)); } catch (ConfigInvalidException | IOException e) { addInvalidNode(); } } } - protected void addExternalDefinitions() throws OrmException { + protected void addSubTasksExternals() throws OrmException { for (String external : task.subTasksExternals) { try { External ext = task.config.getExternal(external); if (ext == null) { addInvalidNode(); } else { - addSubDefinitions(getTaskDefinitions(ext)); + addPreloaded(getPreloadedTasks(ext)); } } catch (ConfigInvalidException | IOException e) { addInvalidNode(); @@ -218,7 +218,7 @@ } } - protected void addSubTasksFactoryDefinitions() throws OrmException { + protected void addSubTasksFactoryTasks() throws ConfigInvalidException, OrmException { for (String tasksFactoryName : task.subTasksFactories) { TasksFactory tasksFactory = task.config.getTasksFactory(tasksFactoryName); if (tasksFactory != null) { @@ -227,10 +227,10 @@ namesFactory = getProperties().getNamesFactory(namesFactory); switch (NamesFactoryType.getNamesFactoryType(namesFactory.type)) { case STATIC: - addStaticTypeTaskDefinitions(tasksFactory, namesFactory); + addStaticTypeTasks(tasksFactory, namesFactory); continue; case CHANGE: - addChangeTypeTaskDefinitions(tasksFactory, namesFactory); + addChangeTypeTasks(tasksFactory, namesFactory); continue; } } @@ -239,15 +239,15 @@ } } - protected void addStaticTypeTaskDefinitions( - TasksFactory tasksFactory, NamesFactory namesFactory) { + protected void addStaticTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) + throws ConfigInvalidException { for (String name : namesFactory.names) { - addSubDefinition(task.config.new Task(tasksFactory, name)); + addPreloaded(preload(task.config.new Task(tasksFactory, name))); } } - protected void addChangeTypeTaskDefinitions( - TasksFactory tasksFactory, NamesFactory namesFactory) { + protected void addChangeTypeTasks(TasksFactory tasksFactory, NamesFactory namesFactory) + throws ConfigInvalidException { try { if (namesFactory.changes != null) { List<ChangeData> changeDataList = @@ -256,8 +256,8 @@ .query(changeQueryBuilderProvider.get().parse(namesFactory.changes)) .entities(); for (ChangeData changeData : changeDataList) { - addSubDefinition( - task.config.new Task(tasksFactory, changeData.getId().toString()), + addPreloaded( + preload(task.config.new Task(tasksFactory, changeData.getId().toString())), (parent, definition) -> new Node(parent, definition) { @Override @@ -275,16 +275,16 @@ addInvalidNode(); } - protected List<Task> getTaskDefinitions(External external) + protected List<Task> getPreloadedTasks(External external) throws ConfigInvalidException, IOException, OrmException { - return getTaskDefinitions(resolveUserBranch(external.user), external.file); + return getPreloadedTasks(resolveUserBranch(external.user), external.file); } - protected List<Task> getTaskDefinitions(Branch.NameKey branch, String file) + protected List<Task> getPreloadedTasks(Branch.NameKey branch, String file) throws ConfigInvalidException, IOException { return taskFactory .getTaskConfig(branch, resolveTaskFileName(file), task.isTrusted) - .getTasks(); + .getPreloadedTasks(); } @Override @@ -293,10 +293,6 @@ } } - protected List<Task> getRootDefinitions() throws ConfigInvalidException, IOException { - return taskFactory.getRootConfig().getRootTasks(); - } - protected String resolveTaskFileName(String file) throws ConfigInvalidException { if (file == null) { throw new ConfigInvalidException("External file not defined"); @@ -319,4 +315,8 @@ } return new Branch.NameKey(allUsers.get(), RefNames.refsUsers(acct.getId())); } + + protected static Task preload(Task task) throws ConfigInvalidException { + return task.config.preloader.preload(task); + } }