AbstractChangeNotes: Lazily create RevWalk

In the common case where there is a ChangeNotesCache hit, we only need
the meta ID from the LoadHandle and not the RevWalk.

Creating RevWalks is cheap, but it's not free. For example, it creates
an ObjectIdOwnerMap which allocates two object arrays of size 1024 and
2048, totaling several KiB. A loaded server may have hundreds of
ChangeNotesCache hits per second, adding up to megabytes per second of
completely unused garbage. This isn't just theoretical: I observed
megabytes of garbage being generated by this path in a live frontend.

Change-Id: I583a3f5cac8b7be4832d49cf8cbdea54b1b3bf8e

java/com/google/gerrit/server/notedb/AbstractChangeNotes.java

@@ -17,7 +17,6 @@ package com.google.gerrit.server.notedb;
 import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
 import static java.util.Objects.requireNonNull;
 
-import com.google.auto.value.AutoValue;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.metrics.Timer1;
@@ -75,31 +74,38 @@ public abstract class AbstractChangeNotes<T> {
     }
   }
 
-  @AutoValue
-  public abstract static class LoadHandle implements AutoCloseable {
-    public static LoadHandle create(ChangeNotesRevWalk walk, ObjectId id) {
+  public static class LoadHandle implements AutoCloseable {
+    private final Repository repo;
+    private final ObjectId id;
+    private ChangeNotesRevWalk rw;
+
+    private LoadHandle(Repository repo, @Nullable ObjectId id) {
+      this.repo = requireNonNull(repo);
+
       if (ObjectId.zeroId().equals(id)) {
         id = null;
       } else if (id != null) {
         id = id.copy();
       }
-      return new AutoValue_AbstractChangeNotes_LoadHandle(requireNonNull(walk), id);
+      this.id = id;
     }
 
-    public static LoadHandle missing() {
-      return new AutoValue_AbstractChangeNotes_LoadHandle(null, null);
+    public ChangeNotesRevWalk walk() {
+      if (rw == null) {
+        rw = ChangeNotesCommit.newRevWalk(repo);
+      }
+      return rw;
     }
 
     @Nullable
-    public abstract ChangeNotesRevWalk walk();
-
-    @Nullable
-    public abstract ObjectId id();
+    public ObjectId id() {
+      return id;
+    }
 
     @Override
     public void close() {
-      if (walk() != null) {
-        walk().close();
+      if (rw != null) {
+        rw.close();
       }
     }
   }
@@ -166,7 +172,7 @@ public abstract class AbstractChangeNotes<T> {
   }
 
   protected LoadHandle openHandle(Repository repo, ObjectId id) {
-    return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), id);
+    return new LoadHandle(repo, id);
   }
 
   public T reload() throws OrmException {

java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -508,7 +508,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     }
 
     ChangeNotesCache.Value v =
-        args.cache.get().get(getProjectName(), getChangeId(), rev, handle.walk());
+        args.cache.get().get(getProjectName(), getChangeId(), rev, handle::walk);
     state = v.state();
     state.copyColumnsTo(change);
     revisionNoteMap = v.revisionNoteMap();

java/com/google/gerrit/server/notedb/ChangeNotesCache.java

@@ -42,6 +42,7 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
+import java.util.function.Supplier;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
 
@@ -335,13 +336,13 @@ public class ChangeNotesCache {
 
   private class Loader implements Callable<ChangeNotesState> {
     private final Key key;
-    private final ChangeNotesRevWalk rw;
+    private final Supplier<ChangeNotesRevWalk> walkSupplier;
 
     private RevisionNoteMap<ChangeRevisionNote> revisionNoteMap;
 
-    private Loader(Key key, ChangeNotesRevWalk rw) {
+    private Loader(Key key, Supplier<ChangeNotesRevWalk> walkSupplier) {
       this.key = key;
-      this.rw = rw;
+      this.walkSupplier = walkSupplier;
     }
 
     @Override
@@ -352,7 +353,7 @@ public class ChangeNotesCache {
           new ChangeNotesParser(
               key.changeId(),
               key.id(),
-              rw,
+              walkSupplier.get(),
               args.changeNoteJson,
               args.legacyChangeNoteRead,
               args.metrics);
@@ -373,11 +374,15 @@ public class ChangeNotesCache {
     this.args = args;
   }
 
-  Value get(Project.NameKey project, Change.Id changeId, ObjectId metaId, ChangeNotesRevWalk rw)
+  Value get(
+      Project.NameKey project,
+      Change.Id changeId,
+      ObjectId metaId,
+      Supplier<ChangeNotesRevWalk> walkSupplier)
       throws IOException {
     try {
       Key key = Key.create(project, changeId, metaId);
-      Loader loader = new Loader(key, rw);
+      Loader loader = new Loader(key, walkSupplier);
       ChangeNotesState s = cache.get(key, loader);
       return new AutoValue_ChangeNotesCache_Value(s, loader.revisionNoteMap);
     } catch (ExecutionException e) {