ChangeNotes.scanChangeIds: Return metaId with ChangeIds
Without this change, ScanOne re-reads the metaRef for each change when
loading its ChangeNotes. With this change, an ls-remote on a replica for
a project with 380K changes stored on NFS improves from around 8 minutes
to around 4 minutes. This also improves offline indexing in some cases.
Change-Id: If1758410a12ee4863d163c30424733e37701aac8
Forward-Compatible: checked
Release-Notes: Improve performance for replica ref advertisement on projects with many changes
diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index da287e9..7a1cd6e 100644
--- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -21,7 +21,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.base.Stopwatch;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.ListenableFuture;
@@ -49,6 +49,7 @@
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ProgressMonitor;
import org.eclipse.jgit.lib.Repository;
@@ -107,11 +108,14 @@
public abstract int slices();
- public abstract ImmutableSet<Change.Id> changeIds();
+ public abstract ImmutableMap<Change.Id, ObjectId> metaIdByChange();
private static ProjectSlice create(
- Project.NameKey name, int slice, int slices, ImmutableSet<Change.Id> changeIds) {
- return new AutoValue_AllChangesIndexer_ProjectSlice(name, slice, slices, changeIds);
+ Project.NameKey name,
+ int slice,
+ int slices,
+ ImmutableMap<Change.Id, ObjectId> metaIdByChange) {
+ return new AutoValue_AllChangesIndexer_ProjectSlice(name, slice, slices, metaIdByChange);
}
}
@@ -192,10 +196,10 @@
Project.NameKey project,
int slice,
int slices,
- ImmutableSet<Change.Id> changeIds,
+ ImmutableMap<Change.Id, ObjectId> metaIdByChange,
Task done,
Task failed) {
- return new ProjectIndexer(indexer, project, slice, slices, changeIds, done, failed);
+ return new ProjectIndexer(indexer, project, slice, slices, metaIdByChange, done, failed);
}
private class ProjectIndexer implements Callable<Void> {
@@ -203,7 +207,7 @@
private final Project.NameKey project;
private final int slice;
private final int slices;
- private final ImmutableSet<Change.Id> changeIds;
+ private final ImmutableMap<Change.Id, ObjectId> metaIdByChange;
private final ProgressMonitor done;
private final ProgressMonitor failed;
@@ -212,14 +216,14 @@
Project.NameKey project,
int slice,
int slices,
- ImmutableSet<Change.Id> changeIds,
+ ImmutableMap<Change.Id, ObjectId> metaIdByChange,
ProgressMonitor done,
ProgressMonitor failed) {
this.indexer = indexer;
this.project = project;
this.slice = slice;
this.slices = slices;
- this.changeIds = changeIds;
+ this.metaIdByChange = metaIdByChange;
this.done = done;
this.failed = failed;
}
@@ -233,7 +237,7 @@
// but the goal is to invalidate that cache as infrequently as we possibly can. And besides,
// we don't have concrete proof that improving packfile locality would help.
notesFactory
- .scan(changeIds, project, id -> (id.get() % slices) == slice)
+ .scan(metaIdByChange, project, id -> (id.get() % slices) == slice)
.forEach(r -> index(r));
OnlineReindexMode.end();
return null;
@@ -338,8 +342,9 @@
@Override
public Void call() throws IOException {
try (Repository repo = repoManager.openRepository(name)) {
- ImmutableSet<Change.Id> changeIds = ChangeNotes.Factory.scanChangeIds(repo);
- int size = changeIds.size();
+ ImmutableMap<Change.Id, ObjectId> metaIdByChange =
+ ChangeNotes.Factory.scanChangeIds(repo);
+ int size = metaIdByChange.size();
if (size > 0) {
changeCount.addAndGet(size);
int slices = 1 + size / PROJECT_SLICE_MAX_REFS;
@@ -352,7 +357,7 @@
projTask.updateTotal(slices);
for (int slice = 0; slice < slices; slice++) {
- ProjectSlice projectSlice = ProjectSlice.create(name, slice, slices, changeIds);
+ ProjectSlice projectSlice = ProjectSlice.create(name, slice, slices, metaIdByChange);
ListenableFuture<?> future =
executor.submit(
reindexProject(
@@ -360,7 +365,7 @@
name,
slice,
slices,
- projectSlice.changeIds(),
+ projectSlice.metaIdByChange(),
doneTask,
failedTask));
String description = "project " + name + " (" + slice + "/" + slices + ")";
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 57070c8..0bcf8fc 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -26,6 +26,7 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableSortedSet;
@@ -69,6 +70,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Predicate;
@@ -113,17 +115,18 @@
this.projectCache = projectCache;
}
- public static ImmutableSet<Change.Id> scanChangeIds(Repository repo) throws IOException {
- ImmutableSet.Builder<Change.Id> fromMeta = ImmutableSet.builder();
+ public static ImmutableMap<Change.Id, ObjectId> scanChangeIds(Repository repo)
+ throws IOException {
+ ImmutableMap.Builder<Change.Id, ObjectId> metaIdByChange = ImmutableMap.builder();
for (Ref r : repo.getRefDatabase().getRefsByPrefix(RefNames.REFS_CHANGES)) {
if (r.getName().endsWith(RefNames.META_SUFFIX)) {
Change.Id id = Change.Id.fromRef(r.getName());
if (id != null) {
- fromMeta.add(id);
+ metaIdByChange.put(id, r.getObjectId());
}
}
}
- return fromMeta.build();
+ return metaIdByChange.build();
}
public ChangeNotes createChecked(Change c) {
@@ -289,23 +292,25 @@
}
public Stream<ChangeNotesResult> scan(
- ImmutableSet<Change.Id> changeIds,
+ ImmutableMap<Change.Id, ObjectId> metaIdByChange,
Project.NameKey project,
Predicate<Change.Id> changeIdPredicate) {
- Stream<Change.Id> idStream = changeIds.stream();
+ Stream<Map.Entry<Change.Id, ObjectId>> metaByIdStream = metaIdByChange.entrySet().stream();
if (changeIdPredicate != null) {
- idStream = idStream.filter(changeIdPredicate);
+ metaByIdStream = metaByIdStream.filter(e -> changeIdPredicate.test(e.getKey()));
}
- return idStream.map(id -> scanOneChange(project, id)).filter(Objects::nonNull);
+ return metaByIdStream.map(e -> scanOneChange(project, e)).filter(Objects::nonNull);
}
@Nullable
- private ChangeNotesResult scanOneChange(Project.NameKey project, Change.Id id) {
+ private ChangeNotesResult scanOneChange(
+ Project.NameKey project, Map.Entry<Change.Id, ObjectId> metaIdByChangeId) {
+ Change.Id id = metaIdByChangeId.getKey();
// TODO(dborowitz): See discussion in BatchUpdate#newChangeContext.
try {
Change change = ChangeNotes.Factory.newChange(project, id);
logger.atFine().log("adding change %s found in project %s", id, project);
- return toResult(change);
+ return toResult(change, metaIdByChangeId.getValue());
} catch (InvalidServerIdException ise) {
logger.atWarning().withCause(ise).log(
"skipping change %d in project %s because of an invalid server id", id.get(), project);
@@ -314,8 +319,8 @@
}
@Nullable
- private ChangeNotesResult toResult(Change rawChangeFromNoteDb) {
- ChangeNotes n = new ChangeNotes(args, rawChangeFromNoteDb, true, null);
+ private ChangeNotesResult toResult(Change rawChangeFromNoteDb, ObjectId metaId) {
+ ChangeNotes n = new ChangeNotes(args, rawChangeFromNoteDb, true, null, metaId);
try {
n.load();
} catch (Exception e) {
