Fix bottleneck in GitRefUpdatedListener
The supermanifest plugin listens on Git ref updates using a class marked as
@Singleton. This means any and all updates to the Git storage will go
through this class. The actual method that handles these updates was
marked synchronized to prevent shared access to a non-thread safe
config.
This turns out to be a central bottleneck in Gerrit: All git updates
have to go through this method. If the supermanifest plugin is configured
to do work (i.e. update manifest entries for other repos), that work is
actually quite slow (5s and sometimes above). This makes it so that updates
on the Gerrit server get to a grinding halt and get all queued up on this
synchronized block.
In this commit, we rework the mutable config to be immutable, remove
the synchronized block and swap it for an AtomicReference. With that,
the machinery is still thread safe, but doesn't lock up anymore.
While at it, we swap a call to ProjectIndex#evictAndReindex for
ProjectIndex#evict. We just want to read a fresh project config
from disk. Eviction is sufficient for that.
Change-Id: I89ffce984d7f4c32dda72dff9e55a5c4c108ee7d
diff --git a/java/com/googlesource/gerrit/plugins/supermanifest/ConfigEntry.java b/java/com/googlesource/gerrit/plugins/supermanifest/ConfigEntry.java
index 82baf05..788d673 100644
--- a/java/com/googlesource/gerrit/plugins/supermanifest/ConfigEntry.java
+++ b/java/com/googlesource/gerrit/plugins/supermanifest/ConfigEntry.java
@@ -17,7 +17,7 @@
import static com.google.common.base.Strings.nullToEmpty;
import static com.google.gerrit.entities.RefNames.REFS_HEADS;
-import com.google.common.collect.Sets;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.entities.Project;
import java.net.URI;
import java.net.URISyntaxException;
@@ -31,19 +31,19 @@
public class ConfigEntry {
public static final String SECTION_NAME = "superproject";
- Project.NameKey srcRepoKey;
- String srcRef;
- URI baseUri;
- ToolType toolType;
- String xmlPath;
- Project.NameKey destRepoKey;
- String repoGroups;
- Set<String> srcRefsExcluded;
- boolean recordSubmoduleLabels;
- boolean ignoreRemoteFailures;
+ final Project.NameKey srcRepoKey;
+ final String srcRef;
+ final URI baseUri;
+ final ToolType toolType;
+ final String xmlPath;
+ final Project.NameKey destRepoKey;
+ final String repoGroups;
+ final ImmutableSet<String> srcRefsExcluded;
+ final boolean recordSubmoduleLabels;
+ final boolean ignoreRemoteFailures;
// destBranch can be "*" in which case srcRef is ignored.
- String destBranch;
+ final String destBranch;
public ConfigEntry(Config cfg, String name) throws ConfigInvalidException {
String[] parts = name.split(":");
@@ -106,7 +106,7 @@
}
srcRefsExcluded =
- Sets.newHashSet(
+ ImmutableSet.copyOf(
Arrays.asList(nullToEmpty(cfg.getString(SECTION_NAME, name, "exclude")).split(",")));
xmlPath = cfg.getString(SECTION_NAME, name, "srcPath");
diff --git a/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java b/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
index 8f685f2..01d789e 100644
--- a/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
+++ b/java/com/googlesource/gerrit/plugins/supermanifest/SuperManifestRefUpdatedListener.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.entities.RefNames.REFS_HEADS;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.errorprone.annotations.FormatMethod;
import com.google.errorprone.annotations.FormatString;
@@ -63,6 +64,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.eclipse.jgit.api.errors.RefNotFoundException;
@@ -104,7 +106,7 @@
private final Timer1<ConfigEntry.ToolType> superprojectCommitTimer;
// Mutable.
- private Set<ConfigEntry> config;
+ private AtomicReference<ImmutableSet<ConfigEntry>> config = new AtomicReference<>();
@Inject
SuperManifestRefUpdatedListener(
@@ -252,12 +254,13 @@
/** for debugging. */
private String configurationToString() {
- if (config == null) {
+ Set<ConfigEntry> cfg = config.get();
+ if (cfg == null) {
return "No config loaded (could not read All-Projects)";
}
StringBuilder b = new StringBuilder();
- b.append("Supermanifest config (").append(config.size()).append(") {\n");
- for (ConfigEntry c : config) {
+ b.append("Supermanifest config (").append(cfg.size()).append(") {\n");
+ for (ConfigEntry c : cfg) {
b.append(" ").append(c).append("\n");
}
b.append("}\n");
@@ -278,17 +281,17 @@
}
}
- config = filtered;
+ config.set(ImmutableSet.copyOf(filtered));
}
@Override
- public synchronized void onGitReferenceUpdated(Event event) {
+ public void onGitReferenceUpdated(Event event) {
if (event.getProjectName().equals(allProjectsName.get())) {
if (event.getRefName().equals("refs/meta/config")) {
try {
// TODO: Remove, this is just band-aid.
// Evict project cache because this is called before that eviction is done in core
- projectCache.evictAndReindex(allProjectsName);
+ projectCache.evict(allProjectsName);
updateConfiguration();
} catch (NoSuchProjectException e) {
throw new IllegalStateException(e);
@@ -334,7 +337,7 @@
identifiedUser.get().getAccountId().get(),
configurationToString());
- if (config == null) {
+ if (config.get() == null) {
error(
"Plugin could not read conf from All-Projects (processing %s:%s)",
manifestProject, manifestBranch);
@@ -364,11 +367,12 @@
}
private List<ConfigEntry> findRelevantConfigs(String project, String refName) {
+ Set<ConfigEntry> cfg = config.get();
List<ConfigEntry> relevantConfigs = new ArrayList<>();
- if (config == null) {
+ if (cfg == null) {
return relevantConfigs;
}
- for (ConfigEntry c : config) {
+ for (ConfigEntry c : cfg) {
if (!c.srcRepoKey.get().equals(project)) {
continue;
}
