RebuildNoteDb: Organize by project

Repeated reopening and closing large projects was causing heap
thrashing due to paging stuff in and out of JGit's internal window
cache. This problem persisted even when preopening the repos to get
them in JGit's internal RepositoryCache.

We can simplify the code and avoid heap thrashing by just rebuilding
one project at a time in each thread. This way we can also reuse a
single NoteDbUpdateManager to make the ref update batching behavior
more transparent.

This does not completely eliminate performance issues, particularly on
spinning disk, but as I write this I am rebuilding AOSP with no
particular CPU thrashing issues.

Change-Id: I2250eb01c8e57e690138dc38ee2cf12b8d0b3aa7

gerrit-pgm/src/main/java/com/google/gerrit/pgm/RebuildNoteDb.java

@@ -17,18 +17,17 @@ package com.google.gerrit.pgm;
 import static com.google.gerrit.server.schema.DataSourceProvider.Context.MULTI_USER;
 
 import com.google.common.base.Function;
+import com.google.common.base.Predicates;
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Ordering;
-import com.google.common.util.concurrent.AsyncFunction;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
-import com.google.gerrit.common.FormatUtil;
 import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.lifecycle.LifecycleManager;
@@ -42,8 +41,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.MultiProgressMonitor;
-import com.google.gerrit.server.git.MultiProgressMonitor.Task;
 import com.google.gerrit.server.git.SearchingChangeCacheImpl;
 import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.index.DummyIndexModule;
@@ -57,7 +54,6 @@ import com.google.inject.AbstractModule;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
 
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.NullProgressMonitor;
@@ -75,9 +71,9 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicBoolean;
 
 public class RebuildNoteDb extends SiteProgram {
   private static final Logger log =
@@ -142,58 +138,46 @@ public class RebuildNoteDb extends SiteProgram {
     ListeningExecutorService executor = newExecutor();
     System.out.println("Rebuilding the NoteDb");
 
-    Multimap<Project.NameKey, Change.Id> changesByProject =
+    final ImmutableMultimap<Project.NameKey, Change.Id> changesByProject =
         getChangesByProject();
-    AtomicBoolean ok = new AtomicBoolean(true);
+    boolean ok;
     Stopwatch sw = Stopwatch.createStarted();
     try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) {
       deleteRefs(RefNames.REFS_DRAFT_COMMENTS, allUsersRepo);
+
+      List<ListenableFuture<Boolean>> futures = new ArrayList<>();
       List<Project.NameKey> projectNames = Ordering.usingToString()
           .sortedCopy(changesByProject.keySet());
-      for (Project.NameKey project : projectNames) {
-        try {
-          List<ListenableFuture<?>> futures = Lists.newArrayList();
-
-          // Here, we elide the project name to 50 characters to ensure that
-          // the whole monitor line for a project fits on one line (<80 chars).
-          final MultiProgressMonitor mpm = new MultiProgressMonitor(System.out,
-              FormatUtil.elide(project.get(), 50));
-          Task doneTask =
-              mpm.beginSubTask("done", changesByProject.get(project).size());
-          Task failedTask =
-              mpm.beginSubTask("failed", MultiProgressMonitor.UNKNOWN);
-
-          for (Change.Id id : changesByProject.get(project)) {
-            // TODO(dborowitz): This can fail if the project no longer exists.
-            // We might not want to just skip conversion of those changes, and
-            // instead move them somewhere like a special lost+found repo.
-            ListenableFuture<?> future = rebuilder.rebuildAsync(id, executor);
-            futures.add(future);
-            future.addListener(
-                new RebuildListener(id, future, ok, doneTask, failedTask),
-                MoreExecutors.directExecutor());
-          }
-
-          mpm.waitFor(Futures.transformAsync(Futures.successfulAsList(futures),
-              new AsyncFunction<List<?>, Void>() {
-                @Override
-                public ListenableFuture<Void> apply(List<?> input) {
-                  mpm.end();
-                  return Futures.immediateFuture(null);
+      for (final Project.NameKey project : projectNames) {
+        ListenableFuture<Boolean> future = executor.submit(
+            new Callable<Boolean>() {
+              @Override
+              public Boolean call() {
+                try (ReviewDb db = unwrap(schemaFactory.open())) {
+                  return rebuilder.rebuildProject(
+                      db, changesByProject, project, allUsersRepo);
+                } catch (Exception e) {
+                  log.error("Error rebuilding project " + project, e);
+                  return false;
                 }
-              }));
-        } catch (Exception e) {
-          log.error("Error rebuilding NoteDb", e);
-          ok.set(false);
-          break;
-        }
+              }
+            });
+        futures.add(future);
+      }
+
+      try {
+        ok = Iterables.all(
+            Futures.allAsList(futures).get(), Predicates.equalTo(true));
+      } catch (InterruptedException | ExecutionException e) {
+        log.error("Error rebuilding projects", e);
+        ok = false;
       }
     }
 
     double t = sw.elapsed(TimeUnit.MILLISECONDS) / 1000d;
     System.out.format("Rebuild %d changes in %.01fs (%.01f/s)\n",
         changesByProject.size(), t, changesByProject.size() / t);
-    return ok.get() ? 0 : 1;
+    return ok ? 0 : 1;
   }
 
   private static void execute(BatchRefUpdate bru, Repository repo)
@@ -243,7 +227,7 @@ public class RebuildNoteDb extends SiteProgram {
     }
   }
 
-  private Multimap<Project.NameKey, Change.Id> getChangesByProject()
+  private ImmutableMultimap<Project.NameKey, Change.Id> getChangesByProject()
       throws OrmException {
     // Memorize all changes so we can close the db connection and allow
     // rebuilder threads to use the full connection pool.
@@ -277,7 +261,7 @@ public class RebuildNoteDb extends SiteProgram {
           }
         }
       }
-      return changesByProject;
+      return ImmutableMultimap.copyOf(changesByProject);
     }
   }
 
@@ -287,63 +271,4 @@ public class RebuildNoteDb extends SiteProgram {
     }
     return db;
   }
-
-  private static class RebuildListener implements Runnable {
-    private Change.Id changeId;
-    private ListenableFuture<?> future;
-    private AtomicBoolean ok;
-    private Task doneTask;
-    private Task failedTask;
-
-    private RebuildListener(Change.Id changeId, ListenableFuture<?> future,
-        AtomicBoolean ok, Task doneTask, Task failedTask) {
-      this.changeId = changeId;
-      this.future = future;
-      this.ok = ok;
-      this.doneTask = doneTask;
-      this.failedTask = failedTask;
-    }
-
-    @Override
-    public void run() {
-      try {
-        future.get();
-        doneTask.update(1);
-      } catch (ExecutionException | InterruptedException e) {
-        if (e.getCause() instanceof RepositoryNotFoundException) {
-          noRepo((RepositoryNotFoundException) e.getCause());
-        } else {
-          fail(e);
-        }
-      } catch (RuntimeException e) {
-        failAndThrow(e);
-      } catch (Error e) {
-        // Can't join with RuntimeException because "RuntimeException
-        // | Error" becomes Throwable, which messes with signatures.
-        failAndThrow(e);
-      }
-    }
-
-    private void fail(Throwable t) {
-      log.error("Failed to rebuild change " + changeId, t);
-      ok.set(false);
-      failedTask.update(1);
-    }
-
-    private void noRepo(RepositoryNotFoundException e) {
-      log.warn("Skipped rebuilding change " + changeId + ": " + e.getMessage());
-      // Don't flip ok bit.
-      failedTask.update(1);
-    }
-
-    private void failAndThrow(RuntimeException e) {
-      fail(e);
-      throw e;
-    }
-
-    private void failAndThrow(Error e) {
-      fail(e);
-      throw e;
-    }
-  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java

@@ -14,15 +14,18 @@
 
 package com.google.gerrit.server.notedb;
 
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
 
 import java.io.IOException;
 import java.util.concurrent.Callable;
@@ -53,4 +56,10 @@ public abstract class ChangeRebuilder {
   public abstract NoteDbChangeState rebuild(NoteDbUpdateManager manager,
       ChangeBundle bundle) throws NoSuchChangeException, IOException,
       OrmException, ConfigInvalidException;
+
+  public abstract boolean rebuildProject(ReviewDb db,
+      ImmutableMultimap<Project.NameKey, Change.Id> allChanges,
+      Project.NameKey project, Repository allUsersRepo)
+      throws NoSuchChangeException, IOException, OrmException,
+      ConfigInvalidException;
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java

@@ -28,12 +28,14 @@ import com.google.common.base.Splitter;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ComparisonChain;
 import com.google.common.collect.FluentIterable;
+import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Sets;
 import com.google.common.primitives.Ints;
+import com.google.gerrit.common.FormatUtil;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -42,6 +44,7 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchLineComment.Status;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.GerritPersonIdent;
@@ -61,13 +64,19 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.errors.InvalidObjectIdException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.TextProgressMonitor;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.transport.ReceiveCommand;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.io.PrintWriter;
 import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -79,6 +88,9 @@ import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 public class ChangeRebuilderImpl extends ChangeRebuilder {
+  private static final Logger log =
+      LoggerFactory.getLogger(ChangeRebuilderImpl.class);
+
   /**
    * The maximum amount of time between the ReviewDb timestamp of the first and
    * last events batched together into a single NoteDb update.
@@ -178,6 +190,38 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
         change, manager.stage().get(change.getId()));
   }
 
+  @Override
+  public boolean rebuildProject(ReviewDb db,
+      ImmutableMultimap<Project.NameKey, Change.Id> allChanges,
+      Project.NameKey project, Repository allUsersRepo)
+      throws NoSuchChangeException, IOException, OrmException,
+      ConfigInvalidException {
+    checkArgument(allChanges.containsKey(project));
+    boolean ok = true;
+    ProgressMonitor pm = new TextProgressMonitor(new PrintWriter(System.out));
+    NoteDbUpdateManager manager = updateManagerFactory.create(project);
+    pm.beginTask(
+        FormatUtil.elide(project.get(), 50), allChanges.get(project).size());
+    try (ObjectInserter allUsersInserter = allUsersRepo.newObjectInserter();
+        RevWalk allUsersRw = new RevWalk(allUsersInserter.newReader())) {
+      manager.setAllUsersRepo(allUsersRepo, allUsersRw, allUsersInserter,
+          new ChainedReceiveCommands());
+      for (Change.Id changeId : allChanges.get(project)) {
+        try {
+          buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
+        } catch (Throwable t) {
+          log.error("Failed to rebuild change " + changeId, t);
+          ok = false;
+        }
+        pm.update(1);
+      }
+      manager.execute();
+    } finally {
+      pm.endTask();
+    }
+    return ok;
+  }
+
   private void buildUpdates(NoteDbUpdateManager manager, ChangeBundle bundle)
       throws IOException, OrmException, ConfigInvalidException {
     Change change = new Change(bundle.getChange());

gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java

@@ -14,13 +14,17 @@
 
 package com.google.gerrit.server.notedb;
 
+import com.google.common.collect.ImmutableMultimap;
 import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Change.Id;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Repository;
 
 import java.io.IOException;
 
@@ -61,6 +65,14 @@ public class NoteDbModule extends FactoryModule {
             OrmException, ConfigInvalidException {
           return null;
         }
+
+        @Override
+        public boolean rebuildProject(ReviewDb db,
+            ImmutableMultimap<NameKey, Id> allChanges, NameKey project,
+            Repository allUsersRepo) throws NoSuchChangeException, IOException,
+            OrmException, ConfigInvalidException {
+          return false;
+        }
       });
     }
   }