Don't require ChangeNotes when rebuilding changes

Previously, ChangeRebuilderImpl was creating new ChangeNotes to pass
to each of its ChangeUpdates. Unfortunately, eager loading of
ChangeNotes means that rebuilding changes with a large number of
events exhibits quadratic behavior, as all N-1 commits in the meta
history were being read in order to produce the Nth commit.

It turns out we don't really need a ChangeNotes in the ChangeUpdate at
all, so we can skip this expensive re-parsing step. This should
improve rebuild speed noticeably.

Change-Id: Icc942dba25fb29dddcdf0ba5411147e63fc779df

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

@@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
@@ -43,10 +44,12 @@ public abstract class AbstractChangeUpdate {
   protected final NotesMigration migration;
   protected final ChangeNoteUtil noteUtil;
   protected final String anonymousCowardName;
-  protected final ChangeNotes notes;
   protected final Account.Id accountId;
   protected final PersonIdent authorIdent;
   protected final Date when;
+
+  @Nullable private final ChangeNotes notes;
+  private final Change change;
   private final PersonIdent serverIdent;
 
   protected PatchSet.Id psId;
@@ -59,15 +62,16 @@ public abstract class AbstractChangeUpdate {
       String anonymousCowardName,
       ChangeNoteUtil noteUtil,
       Date when) {
-    this(
-        migration,
-        noteUtil,
-        serverIdent,
-        anonymousCowardName,
-        ctl.getNotes(),
-        accountId(ctl.getUser()),
-        ident(noteUtil, serverIdent, anonymousCowardName, ctl.getUser(), when),
-        when);
+    this.migration = migration;
+    this.noteUtil = noteUtil;
+    this.serverIdent = new PersonIdent(serverIdent, when);
+    this.anonymousCowardName = anonymousCowardName;
+    this.notes = ctl.getNotes();
+    this.change = notes.getChange();
+    this.accountId = accountId(ctl.getUser());
+    this.authorIdent =
+        ident(noteUtil, serverIdent, anonymousCowardName, ctl.getUser(), when);
+    this.when = when;
   }
 
   protected AbstractChangeUpdate(
@@ -75,15 +79,21 @@ public abstract class AbstractChangeUpdate {
       ChangeNoteUtil noteUtil,
       PersonIdent serverIdent,
       String anonymousCowardName,
-      ChangeNotes notes,
+      @Nullable ChangeNotes notes,
+      @Nullable Change change,
       Account.Id accountId,
       PersonIdent authorIdent,
       Date when) {
+    checkArgument(
+        (notes != null && change == null)
+            || (notes == null && change != null),
+        "exactly one of notes or change required");
     this.migration = migration;
     this.noteUtil = noteUtil;
     this.serverIdent = new PersonIdent(serverIdent, when);
     this.anonymousCowardName = anonymousCowardName;
     this.notes = notes;
+    this.change = change != null ? change : notes.getChange();
     this.accountId = accountId;
     this.authorIdent = authorIdent;
     this.when = when;
@@ -114,15 +124,16 @@ public abstract class AbstractChangeUpdate {
   }
 
   public Change.Id getId() {
-    return notes.getChangeId();
+    return change.getId();
   }
 
+  @Nullable
   public ChangeNotes getNotes() {
     return notes;
   }
 
   public Change getChange() {
-    return notes.getChange();
+    return change;
   }
 
   public Date getWhen() {

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

@@ -21,6 +21,7 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.Sets;
 import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
@@ -62,6 +63,8 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
   public interface Factory {
     ChangeDraftUpdate create(ChangeNotes notes, Account.Id accountId,
         PersonIdent authorIdent, Date when);
+    ChangeDraftUpdate create(Change change, Account.Id accountId,
+        PersonIdent authorIdent, Date when);
   }
 
   @AutoValue
@@ -76,8 +79,8 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
 
   private final AllUsersName draftsProject;
 
-  private List<PatchLineComment> put;
-  private Set<Key> delete;
+  private List<PatchLineComment> put = new ArrayList<>();
+  private Set<Key> delete = new HashSet<>();
 
   @AssistedInject
   private ChangeDraftUpdate(
@@ -90,11 +93,25 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
       @Assisted Account.Id accountId,
       @Assisted PersonIdent authorIdent,
       @Assisted Date when) {
-    super(migration, noteUtil, serverIdent, anonymousCowardName, notes,
+    super(migration, noteUtil, serverIdent, anonymousCowardName, notes, null,
+        accountId, authorIdent, when);
+    this.draftsProject = allUsers;
+  }
+
+  @AssistedInject
+  private ChangeDraftUpdate(
+      @GerritPersonIdent PersonIdent serverIdent,
+      @AnonymousCowardName String anonymousCowardName,
+      NotesMigration migration,
+      AllUsersName allUsers,
+      ChangeNoteUtil noteUtil,
+      @Assisted Change change,
+      @Assisted Account.Id accountId,
+      @Assisted PersonIdent authorIdent,
+      @Assisted Date when) {
+    super(migration, noteUtil, serverIdent, anonymousCowardName, null, change,
         accountId, authorIdent, when);
     this.draftsProject = allUsers;
-    this.put = new ArrayList<>();
-    this.delete = new HashSet<>();
   }
 
   public void putComment(PatchLineComment c) {
@@ -179,14 +196,17 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
       // If reading from changes is enabled, then the old DraftCommentNotes
       // already parsed the revision notes. We can reuse them as long as the ref
       // hasn't advanced.
-      DraftCommentNotes draftNotes =
-          getNotes().load().getDraftCommentNotes();
-      if (draftNotes != null) {
-        ObjectId idFromNotes =
-            firstNonNull(draftNotes.getRevision(), ObjectId.zeroId());
-        RevisionNoteMap rnm = draftNotes.getRevisionNoteMap();
-        if (idFromNotes.equals(curr) && rnm != null) {
-          return rnm;
+      ChangeNotes changeNotes = getNotes();
+      if (changeNotes != null) {
+        DraftCommentNotes draftNotes =
+            changeNotes.load().getDraftCommentNotes();
+        if (draftNotes != null) {
+          ObjectId idFromNotes =
+              firstNonNull(draftNotes.getRevision(), ObjectId.zeroId());
+          RevisionNoteMap rnm = draftNotes.getRevisionNoteMap();
+          if (idFromNotes.equals(curr) && rnm != null) {
+            return rnm;
+          }
         }
       }
     }

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

@@ -116,7 +116,6 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
 
   private final AccountCache accountCache;
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
-  private final ChangeNotes.Factory notesFactory;
   private final ChangeNoteUtil changeNoteUtil;
   private final ChangeUpdate.Factory updateFactory;
   private final NoteDbUpdateManager.Factory updateManagerFactory;
@@ -129,7 +128,6 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   ChangeRebuilderImpl(SchemaFactory<ReviewDb> schemaFactory,
       AccountCache accountCache,
       ChangeDraftUpdate.Factory draftUpdateFactory,
-      ChangeNotes.Factory notesFactory,
       ChangeNoteUtil changeNoteUtil,
       ChangeUpdate.Factory updateFactory,
       NoteDbUpdateManager.Factory updateManagerFactory,
@@ -140,7 +138,6 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     super(schemaFactory);
     this.accountCache = accountCache;
     this.draftUpdateFactory = draftUpdateFactory;
-    this.notesFactory = notesFactory;
     this.changeNoteUtil = changeNoteUtil;
     this.updateFactory = updateFactory;
     this.updateManagerFactory = updateManagerFactory;
@@ -388,8 +385,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
       labelNameComparator = Ordering.natural();
     }
     ChangeUpdate update = updateFactory.create(
-        notesFactory.createWithAutoRebuildingDisabled(
-            change, manager.getChangeRepo().cmds),
+        change,
         events.getAccountId(),
         events.newAuthorIdent(),
         events.getWhen(),
@@ -406,13 +402,12 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
 
   private void flushEventsToDraftUpdate(NoteDbUpdateManager manager,
       EventList<PatchLineCommentEvent> events, Change change)
-      throws OrmException, IOException {
+      throws OrmException {
     if (events.isEmpty()) {
       return;
     }
     ChangeDraftUpdate update = draftUpdateFactory.create(
-        notesFactory.createWithAutoRebuildingDisabled(
-            change, manager.getChangeRepo().cmds),
+        change,
         events.getAccountId(),
         events.newAuthorIdent(),
         events.getWhen());

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

@@ -95,7 +95,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   public interface Factory {
     ChangeUpdate create(ChangeControl ctl);
     ChangeUpdate create(ChangeControl ctl, Date when);
-    ChangeUpdate create(ChangeNotes notes, @Nullable Account.Id accountId,
+    ChangeUpdate create(Change change, @Nullable Account.Id accountId,
         PersonIdent authorIdent, Date when,
         Comparator<String> labelNameComparator);
 
@@ -204,12 +204,12 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       NoteDbUpdateManager.Factory updateManagerFactory,
       ChangeDraftUpdate.Factory draftUpdateFactory,
       ChangeNoteUtil noteUtil,
-      @Assisted ChangeNotes notes,
+      @Assisted Change change,
       @Assisted @Nullable Account.Id accountId,
       @Assisted PersonIdent authorIdent,
       @Assisted Date when,
       @Assisted Comparator<String> labelNameComparator) {
-    super(migration, noteUtil, serverIdent, anonymousCowardName, notes,
+    super(migration, noteUtil, serverIdent, anonymousCowardName, null, change,
         accountId, authorIdent, when);
     this.accountCache = accountCache;
     this.draftUpdateFactory = draftUpdateFactory;
@@ -328,8 +328,14 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   @VisibleForTesting
   ChangeDraftUpdate createDraftUpdateIfNull() {
     if (draftUpdate == null) {
-      draftUpdate =
-          draftUpdateFactory.create(getNotes(), accountId, authorIdent, when);
+      ChangeNotes notes = getNotes();
+      if (notes != null) {
+        draftUpdate =
+            draftUpdateFactory.create(notes, accountId, authorIdent, when);
+      } else {
+        draftUpdate = draftUpdateFactory.create(
+            getChange(), accountId, authorIdent, when);
+      }
     }
     return draftUpdate;
   }
@@ -428,10 +434,13 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       // If reading from changes is enabled, then the old ChangeNotes already
       // parsed the revision notes. We can reuse them as long as the ref hasn't
       // advanced.
-      ObjectId idFromNotes =
-          firstNonNull(getNotes().load().getRevision(), ObjectId.zeroId());
-      if (idFromNotes.equals(curr)) {
-        return checkNotNull(getNotes().revisionNoteMap);
+      ChangeNotes notes = getNotes();
+      if (notes != null) {
+        ObjectId idFromNotes =
+            firstNonNull(notes.load().getRevision(), ObjectId.zeroId());
+        if (idFromNotes.equals(curr)) {
+          return checkNotNull(getNotes().revisionNoteMap);
+        }
       }
     }
     NoteMap noteMap = NoteMap.read(rw.getObjectReader(), rw.parseCommit(curr));