ProjectLevelConfig: Cache parsed config and avoid reparsing

ProjectLevelConfig contained multiple inefficiencies:
1) In CachedProjectConfig we only cached a String that we parsed
   dynamically when asked for a project level config.
2) When resolving a ProjectLevelConfig with inheritance, we yet
   again made mutable copies by calling Config#toText/fromText.

This made it so that for a ProjectLevelConfig requested at level N
in the project hierarchy, we called Config#{to,from}Text 4*N times.

code-owners calls requests a ProjectLevelConfig quite frequently
while processing. This is something we can look at serparately,
but it exemplified the problem.

Change-Id: I58e3fed45cd2685cce47f2acc94f70de93b1845a

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 @@ import org.eclipse.jgit.lib.ObjectId;
  */
 @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 class CachedProjectConfig {
 
   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 @@ public abstract class CachedProjectConfig {
 
     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;
     }
 

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();
+  }
+}

java/com/google/gerrit/server/project/ProjectLevelConfig.java

@@ -16,14 +16,15 @@ package com.google.gerrit.server.project;
 
 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 @@ public class ProjectLevelConfig {
 
   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) {
+    return immutableConfig.mutableCopy();
-      cfg = new Config();
-    }
-    return cfg;
   }
 
   public Config getWithInheritance() {
@@ -105,58 +104,54 @@ public class ProjectLevelConfig {
    * @return a combined config.
    */
   public Config getWithInheritance(boolean merge) {
-    Config cfgWithInheritance = new Config();
+    Config cfg = new Config();
-    try {
+    // Traverse from All-Projects down to the current project
-      cfgWithInheritance.fromText(get().toText());
+    StreamSupport.stream(project.treeInOrder().spliterator(), false)
-    } catch (ConfigInvalidException e) {
+        .forEach(
-      // cannot happen
+            parent -> {
-    }
+              ImmutableConfig levelCfg = parent.getConfig(fileName).immutableConfig;
-    ProjectState parent = Iterables.getFirst(project.parents(), null);
+              for (String section : levelCfg.getSections()) {
-    if (parent != null) {
+                Set<String> allNames = cfg.getNames(section);
-      Config parentCfg = parent.getConfig(fileName).getWithInheritance();
+                for (String name : levelCfg.getNames(section)) {
-      for (String section : parentCfg.getSections()) {
+                  String[] levelValues = levelCfg.getStringList(section, null, name);
-        Set<String> allNames = get().getNames(section);
+                  if (allNames.contains(name) && merge) {
-        for (String name : parentCfg.getNames(section)) {
+                    cfg.setStringList(
-          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))
+                                Arrays.stream(levelValues))
                             .sorted()
                             .distinct()
                             .collect(toList()));
+                  } else {
+                    cfg.setStringList(section, null, name, Arrays.asList(levelValues));
                   }
                 }
 
-        for (String subsection : parentCfg.getSubsections(section)) {
+                for (String subsection : levelCfg.getSubsections(section)) {
-          allNames = get().getNames(section, subsection);
+                  allNames = cfg.getNames(section, subsection);
-          for (String name : parentCfg.getNames(section, subsection)) {
+                  for (String name : levelCfg.getNames(section, subsection)) {
-            String[] parentValues = parentCfg.getStringList(section, subsection, name);
+                    String[] levelValues = levelCfg.getStringList(section, subsection, name);
-            if (!allNames.contains(name)) {
+                    if (allNames.contains(name) && merge) {
-              cfgWithInheritance.setStringList(
+                      cfg.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))
+                                  Arrays.stream(levelValues))
                               .sorted()
                               .distinct()
                               .collect(toList()));
+                    } else {
+                      cfg.setStringList(section, subsection, name, Arrays.asList(levelValues));
                     }
                   }
                 }
               }
-    }
+            });
-    return cfgWithInheritance;
+    return cfg;
   }
 }

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 class ProjectState {
   }
 
   public ProjectLevelConfig getConfig(String fileName) {
-    Optional<Config> rawConfig = cachedConfig.getProjectLevelConfig(fileName);
+    checkState(fileName.endsWith(".config"), "file name must end in .config. is: " + fileName);
-    return new ProjectLevelConfig(fileName, this, rawConfig.orElse(new Config()));
+    return new ProjectLevelConfig(
+        fileName, this, cachedConfig.getParsedProjectLevelConfigs().get(fileName));
   }
 
   public long getMaxObjectSizeLimit() {