Cache parsed ChangeNotes

By instrumenting a dev server, I found that loading the change screen
for a single change plus 2 related changes and 2 conflicting changes
results in a whopping 25 loads of a ChangeNotes. On a SQL database
that's not too terrible, but git storage is enough slower and more
memory-intensive than SQL that this is noticeable. Plus, the
ChangeNotes approach is to parse EVERYTHING, which is often more than
we need.

Solve this problem in the way we solve all problems: throw a cache in
front of it. This data has the nice property that the entities parsed
from a particular SHA-1 are immutable forever, so we don't need to
worry about invalidation like we would with a cache keyed by, say,
change number.

Change-Id: I4bbb9bf82f8f228b3ae8333a288af35d163c050f

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

@@ -51,10 +51,14 @@ public abstract class AbstractChangeNotes<T> {
     final NoteDbMetrics metrics;
     final Provider<ReviewDb> db;
 
-    // Must be a Provider due to dependency cycle between ChangeRebuilder and
-    // Args via ChangeNotes.Factory.
+    // Providers required to avoid dependency cycles.
+
+    // ChangeRebuilder -> ChangeNotes.Factory -> Args
     final Provider<ChangeRebuilder> rebuilder;
 
+    // ChangeNoteCache -> Args
+    final Provider<ChangeNotesCache> cache;
+
     @Inject
     Args(
         GitRepositoryManager repoManager,
@@ -63,7 +67,8 @@ public abstract class AbstractChangeNotes<T> {
         ChangeNoteUtil noteUtil,
         NoteDbMetrics metrics,
         Provider<ReviewDb> db,
-        Provider<ChangeRebuilder> rebuilder) {
+        Provider<ChangeRebuilder> rebuilder,
+        Provider<ChangeNotesCache> cache) {
       this.repoManager = repoManager;
       this.migration = migration;
       this.allUsers = allUsers;
@@ -71,6 +76,7 @@ public abstract class AbstractChangeNotes<T> {
       this.metrics = metrics;
       this.db = db;
       this.rebuilder = rebuilder;
+      this.cache = cache;
     }
   }
 

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

@@ -533,11 +533,12 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       loadDefaults();
       return;
     }
-    ChangeNotesParser parser = new ChangeNotesParser(
-        change.getId(), rev, handle.walk(), args.noteUtil, args.metrics);
-    state = parser.parseAll();
+
+    ChangeNotesCache.Value v = args.cache.get().get(
+        getProjectName(), getChangeId(), rev, handle.walk());
+    state = v.state();
     state.copyColumnsTo(change);
-    revisionNoteMap = parser.getRevisionNoteMap();
+    revisionNoteMap = v.revisionNoteMap();
   }
 
   @Override

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

@@ -0,0 +1,128 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.cache.Cache;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.notedb.AbstractChangeNotes.Args;
+import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import com.google.inject.Singleton;
+import com.google.inject.name.Named;
+
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.ObjectId;
+
+import java.io.IOException;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+
+@Singleton
+class ChangeNotesCache {
+  @VisibleForTesting
+  static final String CACHE_NAME = "change_notes";
+
+  static Module module() {
+    return new CacheModule() {
+      @Override
+      protected void configure() {
+        bind(ChangeNotesCache.class);
+        cache(CACHE_NAME,
+            Key.class,
+            ChangeNotesState.class)
+          .maximumWeight(1000);
+      }
+    };
+  }
+
+  @AutoValue
+  static abstract class Key {
+    abstract Project.NameKey project();
+    abstract Change.Id changeId();
+    abstract ObjectId id();
+  }
+
+  @AutoValue
+  static abstract class Value {
+    abstract ChangeNotesState state();
+
+    /**
+     * The {@link RevisionNoteMap} produced while parsing this change.
+     * <p>
+     * These instances are mutable and non-threadsafe, so it is only safe to
+     * return it to the caller that actually incurred the cache miss. It is only
+     * used as an optimization; {@link ChangeNotes} is capable of lazily loading
+     * it as necessary.
+     */
+    @Nullable abstract RevisionNoteMap revisionNoteMap();
+  }
+
+  private class Loader implements Callable<ChangeNotesState> {
+    private final Key key;
+    private final ChangeNotesRevWalk rw;
+
+    private RevisionNoteMap revisionNoteMap;
+
+    private Loader(Key key, ChangeNotesRevWalk rw) {
+      this.key = key;
+      this.rw = rw;
+    }
+
+    @Override
+    public ChangeNotesState call() throws ConfigInvalidException, IOException {
+      ChangeNotesParser parser = new ChangeNotesParser(
+          key.changeId(), key.id(), rw, args.noteUtil, args.metrics);
+      ChangeNotesState result = parser.parseAll();
+      // This assignment only happens if call() was actually called, which only
+      // happens when Cache#get(K, Callable<V>) incurs a cache miss.
+      revisionNoteMap = parser.getRevisionNoteMap();
+      return result;
+    }
+  }
+
+  private final Cache<Key, ChangeNotesState> cache;
+  private final Args args;
+
+  @Inject
+  ChangeNotesCache(
+      @Named(CACHE_NAME) Cache<Key, ChangeNotesState> cache,
+      Args args) {
+    this.cache = cache;
+    this.args = args;
+  }
+
+  Value get(Project.NameKey project, Change.Id changeId,
+      ObjectId metaId, ChangeNotesRevWalk rw) throws IOException {
+    try {
+      Key key =
+          new AutoValue_ChangeNotesCache_Key(project, changeId, metaId.copy());
+      Loader loader = new Loader(key, rw);
+      ChangeNotesState s = cache.get(key, loader);
+      return new AutoValue_ChangeNotesCache_Value(s, loader.revisionNoteMap);
+    } catch (ExecutionException e) {
+      throw new IOException(String.format(
+              "Error loading %s in %s at %s",
+              RefNames.changeMetaRef(changeId), project, metaId.name()),
+          e);
+    }
+  }
+}

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

@@ -431,15 +431,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       return RevisionNoteMap.emptyMap();
     }
     if (migration.readChanges()) {
-      // 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.
+      // If reading from changes is enabled, then the old ChangeNotes may have
+      // already parsed the revision notes. We can reuse them as long as the ref
+      // hasn't advanced.
       ChangeNotes notes = getNotes();
-      if (notes != null) {
+      if (notes != null && notes.revisionNoteMap != null) {
         ObjectId idFromNotes =
             firstNonNull(notes.load().getRevision(), ObjectId.zeroId());
         if (idFromNotes.equals(curr)) {
-          return checkNotNull(getNotes().revisionNoteMap);
+          return notes.revisionNoteMap;
         }
       }
     }

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

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.notedb;
 
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.reviewdb.client.Change;
@@ -22,6 +24,8 @@ 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 com.google.inject.TypeLiteral;
+import com.google.inject.name.Names;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
@@ -53,6 +57,7 @@ public class NoteDbModule extends FactoryModule {
     factory(DraftCommentNotes.Factory.class);
     factory(NoteDbUpdateManager.Factory.class);
     if (!useTestBindings) {
+      install(ChangeNotesCache.module());
       if (cfg.getBoolean("noteDb", null, "testRebuilderWrapper", false)) {
         // Yes, another variety of test bindings with a different way of
         // configuring it.
@@ -83,6 +88,10 @@ public class NoteDbModule extends FactoryModule {
           return false;
         }
       });
+      bind(new TypeLiteral<Cache<ChangeNotesCache.Key, ChangeNotesState>>() {})
+          .annotatedWith(Names.named(ChangeNotesCache.CACHE_NAME))
+          .toInstance(CacheBuilder.newBuilder()
+              .<ChangeNotesCache.Key, ChangeNotesState>build());
     }
   }
 }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -22,8 +22,10 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
+import static org.junit.Assert.fail;
 
 import com.google.common.base.Function;
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMultimap;
@@ -52,6 +54,7 @@ import com.google.gerrit.testutil.TestChanges;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 
+import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
@@ -802,9 +805,15 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     RevCommit commit = tr.commit().message("PS1 again").create();
     update.setCommit(rw, commit);
     update.commit();
-    exception.expect(OrmException.class);
-    exception.expectMessage("Multiple revisions parsed for patch set");
+
+    try {
       notes = newNotes(c);
+      fail("Expected IOException");
+    } catch (OrmException e) {
+      assertCause(e, ConfigInvalidException.class,
+          "Multiple revisions parsed for patch set 1:"
+              + " RevId{" + commit.name() + "} and " + ps.getRevision().get());
+    }
   }
 
   @Test
@@ -2285,4 +2294,20 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
       return ref != null ? ref.getObjectId() : null;
     }
   }
+
+  private void assertCause(Throwable e,
+      Class<? extends Throwable> expectedClass, String expectedMsg) {
+    Throwable cause = null;
+    for (Throwable t : Throwables.getCausalChain(e)) {
+      if (expectedClass.isAssignableFrom(t.getClass())) {
+        cause = t;
+        break;
+      }
+    }
+    assertThat(cause)
+        .named(expectedClass.getSimpleName() + " in causal chain of:\n"
+            + Throwables.getStackTraceAsString(e))
+        .isNotNull();
+    assertThat(cause.getMessage()).isEqualTo(expectedMsg);
+  }
 }