Merge "ProjectLevelConfig: Cache parsed config and avoid reparsing"
diff --git a/java/com/google/gerrit/entities/CachedProjectConfig.java b/java/com/google/gerrit/entities/CachedProjectConfig.java
index 0b755b7..2a94bc8 100644
--- a/java/com/google/gerrit/entities/CachedProjectConfig.java
+++ b/java/com/google/gerrit/entities/CachedProjectConfig.java
@@ -14,19 +14,17 @@
package com.google.gerrit.entities;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.eclipse.jgit.annotations.Nullable;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
/**
@@ -37,6 +35,8 @@
*/
@AutoValue
public abstract class CachedProjectConfig {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
public abstract Project getProject();
public abstract ImmutableMap<AccountGroup.UUID, GroupReference> getGroups();
@@ -126,34 +126,10 @@
public abstract ImmutableMap<String, String> getPluginConfigs();
- /**
- * Returns the {@link Config} that got parsed from the specified {@code fileName} on {@code
- * refs/meta/config}. The returned instance is a defensive copy of the cached value.
- *
- * @param fileName the name of the file. Must end in {@code .config}.
- * @return an {@link Optional} of the {@link Config}. {@link Optional#empty()} if the file was not
- * found or could not be parsed. {@link com.google.gerrit.server.project.ProjectConfig} will
- * surface validation errors in case of a parsing issue.
- */
- public Optional<Config> getProjectLevelConfig(String fileName) {
- checkState(fileName.endsWith(".config"), "file name must end in .config");
- if (getProjectLevelConfigs().containsKey(fileName)) {
- Config config = new Config();
- try {
- config.fromText(getProjectLevelConfigs().get(fileName));
- } catch (ConfigInvalidException e) {
- // This is OK to propagate as IllegalStateException because it's a programmer error.
- // The config was converted to a String using Config#toText. So #fromText must not
- // throw a ConfigInvalidException
- throw new IllegalStateException("invalid config for " + fileName, e);
- }
- return Optional.of(config);
- }
- return Optional.empty();
- }
-
public abstract ImmutableMap<String, String> getProjectLevelConfigs();
+ public abstract ImmutableMap<String, ImmutableConfig> getParsedProjectLevelConfigs();
+
public static Builder builder() {
return new AutoValue_CachedProjectConfig.Builder();
}
@@ -235,8 +211,15 @@
abstract ImmutableMap.Builder<String, String> projectLevelConfigsBuilder();
+ abstract ImmutableMap.Builder<String, ImmutableConfig> parsedProjectLevelConfigsBuilder();
+
public Builder addProjectLevelConfig(String configFileName, String config) {
projectLevelConfigsBuilder().put(configFileName, config);
+ try {
+ parsedProjectLevelConfigsBuilder().put(configFileName, ImmutableConfig.parse(config));
+ } catch (ConfigInvalidException e) {
+ logger.atInfo().withCause(e).log("Config for " + configFileName + " not parsable");
+ }
return this;
}
diff --git a/java/com/google/gerrit/entities/ImmutableConfig.java b/java/com/google/gerrit/entities/ImmutableConfig.java
new file mode 100644
index 0000000..a5efc14
--- /dev/null
+++ b/java/com/google/gerrit/entities/ImmutableConfig.java
@@ -0,0 +1,91 @@
+// Copyright (C) 2021 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.google.gerrit.entities;
+
+import java.util.Set;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
+
+/**
+ * Immutable parsed representation of a {@link org.eclipse.jgit.lib.Config} that can be cached.
+ * Supports only a limited set of operations.
+ */
+public class ImmutableConfig {
+ public static final ImmutableConfig EMPTY = new ImmutableConfig("", new Config());
+
+ private final String stringCfg;
+ private final Config cfg;
+
+ private ImmutableConfig(String stringCfg, Config cfg) {
+ this.stringCfg = stringCfg;
+ this.cfg = cfg;
+ }
+
+ public static ImmutableConfig parse(String stringCfg) throws ConfigInvalidException {
+ Config cfg = new Config();
+ cfg.fromText(stringCfg);
+ return new ImmutableConfig(stringCfg, cfg);
+ }
+
+ /** Returns a mutable copy of this config. */
+ public Config mutableCopy() {
+ Config cfg = new Config();
+ try {
+ cfg.fromText(this.cfg.toText());
+ } catch (ConfigInvalidException e) {
+ // Can't happen as we used JGit to format that config.
+ throw new IllegalStateException(e);
+ }
+ return cfg;
+ }
+
+ /** @see Config#getSections() */
+ public Set<String> getSections() {
+ return cfg.getSections();
+ }
+
+ /** @see Config#getNames(String) */
+ public Set<String> getNames(String section) {
+ return cfg.getNames(section);
+ }
+
+ /** @see Config#getNames(String, String) */
+ public Set<String> getNames(String section, String subsection) {
+ return cfg.getNames(section, subsection);
+ }
+
+ /** @see Config#getStringList(String, String, String) */
+ public String[] getStringList(String section, String subsection, String name) {
+ return cfg.getStringList(section, subsection, name);
+ }
+
+ /** @see Config#getSubsections(String) */
+ public Set<String> getSubsections(String section) {
+ return cfg.getSubsections(section);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof ImmutableConfig)) {
+ return false;
+ }
+ return ((ImmutableConfig) o).stringCfg.equals(stringCfg);
+ }
+
+ @Override
+ public int hashCode() {
+ return stringCfg.hashCode();
+ }
+}
diff --git a/java/com/google/gerrit/server/project/ProjectLevelConfig.java b/java/com/google/gerrit/server/project/ProjectLevelConfig.java
index 4825233..555cf4c 100644
--- a/java/com/google/gerrit/server/project/ProjectLevelConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectLevelConfig.java
@@ -16,14 +16,15 @@
import static java.util.stream.Collectors.toList;
-import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
+import com.google.gerrit.entities.ImmutableConfig;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.server.git.meta.VersionedMetaData;
import java.io.IOException;
import java.util.Arrays;
import java.util.Set;
import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
import org.eclipse.jgit.annotations.Nullable;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
@@ -73,19 +74,17 @@
private final String fileName;
private final ProjectState project;
- private Config cfg;
+ private final ImmutableConfig immutableConfig;
- public ProjectLevelConfig(String fileName, ProjectState project, Config cfg) {
+ public ProjectLevelConfig(
+ String fileName, ProjectState project, @Nullable ImmutableConfig immutableConfig) {
this.fileName = fileName;
this.project = project;
- this.cfg = cfg;
+ this.immutableConfig = immutableConfig == null ? ImmutableConfig.EMPTY : immutableConfig;
}
public Config get() {
- if (cfg == null) {
- cfg = new Config();
- }
- return cfg;
+ return immutableConfig.mutableCopy();
}
public Config getWithInheritance() {
@@ -105,58 +104,54 @@
* @return a combined config.
*/
public Config getWithInheritance(boolean merge) {
- Config cfgWithInheritance = new Config();
- try {
- cfgWithInheritance.fromText(get().toText());
- } catch (ConfigInvalidException e) {
- // cannot happen
- }
- ProjectState parent = Iterables.getFirst(project.parents(), null);
- if (parent != null) {
- Config parentCfg = parent.getConfig(fileName).getWithInheritance();
- for (String section : parentCfg.getSections()) {
- Set<String> allNames = get().getNames(section);
- for (String name : parentCfg.getNames(section)) {
- String[] parentValues = parentCfg.getStringList(section, null, name);
- if (!allNames.contains(name)) {
- cfgWithInheritance.setStringList(section, null, name, Arrays.asList(parentValues));
- } else if (merge) {
- cfgWithInheritance.setStringList(
- section,
- null,
- name,
- Stream.concat(
- Arrays.stream(cfg.getStringList(section, null, name)),
- Arrays.stream(parentValues))
- .sorted()
- .distinct()
- .collect(toList()));
- }
- }
+ Config cfg = new Config();
+ // Traverse from All-Projects down to the current project
+ StreamSupport.stream(project.treeInOrder().spliterator(), false)
+ .forEach(
+ parent -> {
+ ImmutableConfig levelCfg = parent.getConfig(fileName).immutableConfig;
+ for (String section : levelCfg.getSections()) {
+ Set<String> allNames = cfg.getNames(section);
+ for (String name : levelCfg.getNames(section)) {
+ String[] levelValues = levelCfg.getStringList(section, null, name);
+ if (allNames.contains(name) && merge) {
+ cfg.setStringList(
+ section,
+ null,
+ name,
+ Stream.concat(
+ Arrays.stream(cfg.getStringList(section, null, name)),
+ Arrays.stream(levelValues))
+ .sorted()
+ .distinct()
+ .collect(toList()));
+ } else {
+ cfg.setStringList(section, null, name, Arrays.asList(levelValues));
+ }
+ }
- for (String subsection : parentCfg.getSubsections(section)) {
- allNames = get().getNames(section, subsection);
- for (String name : parentCfg.getNames(section, subsection)) {
- String[] parentValues = parentCfg.getStringList(section, subsection, name);
- if (!allNames.contains(name)) {
- cfgWithInheritance.setStringList(
- section, subsection, name, Arrays.asList(parentValues));
- } else if (merge) {
- cfgWithInheritance.setStringList(
- section,
- subsection,
- name,
- Streams.concat(
- Arrays.stream(cfg.getStringList(section, subsection, name)),
- Arrays.stream(parentValues))
- .sorted()
- .distinct()
- .collect(toList()));
- }
- }
- }
- }
- }
- return cfgWithInheritance;
+ for (String subsection : levelCfg.getSubsections(section)) {
+ allNames = cfg.getNames(section, subsection);
+ for (String name : levelCfg.getNames(section, subsection)) {
+ String[] levelValues = levelCfg.getStringList(section, subsection, name);
+ if (allNames.contains(name) && merge) {
+ cfg.setStringList(
+ section,
+ subsection,
+ name,
+ Streams.concat(
+ Arrays.stream(cfg.getStringList(section, subsection, name)),
+ Arrays.stream(levelValues))
+ .sorted()
+ .distinct()
+ .collect(toList()));
+ } else {
+ cfg.setStringList(section, subsection, name, Arrays.asList(levelValues));
+ }
+ }
+ }
+ }
+ });
+ return cfg;
}
}
diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java
index 8c024ef..249eb35 100644
--- a/java/com/google/gerrit/server/project/ProjectState.java
+++ b/java/com/google/gerrit/server/project/ProjectState.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
+import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.entities.PermissionRule.Action.ALLOW;
import static java.util.Comparator.comparing;
@@ -176,8 +177,9 @@
}
public ProjectLevelConfig getConfig(String fileName) {
- Optional<Config> rawConfig = cachedConfig.getProjectLevelConfig(fileName);
- return new ProjectLevelConfig(fileName, this, rawConfig.orElse(new Config()));
+ checkState(fileName.endsWith(".config"), "file name must end in .config. is: " + fileName);
+ return new ProjectLevelConfig(
+ fileName, this, cachedConfig.getParsedProjectLevelConfigs().get(fileName));
}
public long getMaxObjectSizeLimit() {
