Merge changes from topic 'diff-summary-cache'

* changes:
  Weigh DiffSummary by on-heap size, not serialized size
  Version DiffSummary cache separately from PatchList cache
  DiffSummary: Improve serialization
  Rename FileList cache to DiffSummary

Documentation/config-gerrit.txt

@@ -642,7 +642,7 @@ Default is 1024 for most caches, except:
 * `"adv_bases"`: default is `4096`
 * `"diff"`: default is `10m` (10 MiB of memory)
 * `"diff_intraline"`: default is `10m` (10 MiB of memory)
-* `"diff_file_list"`: default is `10m` (10 MiB of memory)
+* `"diff_summary"`: default is `10m` (10 MiB of memory)
 * `"plugin_resources"`: default is 2m (2 MiB of memory)
 
 +
@@ -660,7 +660,7 @@ performed once every 24 hours.
 +
 Default is 128 MiB per cache, except:
 +
-* `"diff_file_list"`: default is `1g` (1 GiB of disk space)
+* `"diff_summary"`: default is `1g` (1 GiB of disk space)
 
 +
 If 0, disk storage for the cache is disabled.
@@ -733,7 +733,7 @@ estimate in bytes of memory used. Administrators should try to target
 cache.diff.memoryLimit to fit all files users will view in a 1 or 2
 day span.
 
-cache `"diff_file_list"`::
+cache `"diff_summary"`::
 +
 Each item caches list of file paths which are different between two
 commits. Gerrit uses this cache to accelerate computing of the list

gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummary.java

@@ -14,16 +14,11 @@
 
 package com.google.gerrit.server.patch;
 
-import static com.google.gerrit.server.ioutil.BasicSerialization.readBytes;
 import static com.google.gerrit.server.ioutil.BasicSerialization.readString;
-import static com.google.gerrit.server.ioutil.BasicSerialization.writeBytes;
+import static com.google.gerrit.server.ioutil.BasicSerialization.readVarInt32;
 import static com.google.gerrit.server.ioutil.BasicSerialization.writeString;
+import static com.google.gerrit.server.ioutil.BasicSerialization.writeVarInt32;
 
-import com.google.common.base.Joiner;
-import com.google.common.base.Splitter;
-
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
@@ -34,12 +29,12 @@ import java.util.List;
 import java.util.zip.DeflaterOutputStream;
 import java.util.zip.InflaterInputStream;
 
-public class FileList implements Serializable {
-  private static final long serialVersionUID = PatchListKey.serialVersionUID;
+public class DiffSummary implements Serializable {
+  private static final long serialVersionUID = DiffSummaryKey.serialVersionUID;
 
   private transient String[] paths;
 
-  public FileList(String[] paths) {
+  public DiffSummary(String[] paths) {
     this.paths = paths;
   }
 
@@ -48,18 +43,20 @@ public class FileList implements Serializable {
   }
 
   private void writeObject(ObjectOutputStream output) throws IOException {
-    ByteArrayOutputStream buf = new ByteArrayOutputStream();
-    try (DeflaterOutputStream out = new DeflaterOutputStream(buf)) {
-      writeString(out, Joiner.on('\n').join(paths));
+    writeVarInt32(output, paths.length);
+    try (DeflaterOutputStream out = new DeflaterOutputStream(output)) {
+      for (String p : paths) {
+        writeString(out, p);
+      }
     }
-    writeBytes(output, buf.toByteArray());
   }
 
   private void readObject(ObjectInputStream input) throws IOException {
-    ByteArrayInputStream buf = new ByteArrayInputStream(readBytes(input));
-    try (InflaterInputStream in = new InflaterInputStream(buf)) {
-      List<String> l = Splitter.on('\n').splitToList(readString(in));
-      paths = l.toArray(new String[l.size()]);
+    paths = new String[readVarInt32(input)];
+    try (InflaterInputStream in = new InflaterInputStream(input)) {
+      for (int i = 0; i < paths.length; i++) {
+        paths[i] = readString(in);
+      }
     }
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java

@@ -0,0 +1,117 @@
+// 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.patch;
+
+import static org.eclipse.jgit.lib.ObjectIdSerialization.readCanBeNull;
+import static org.eclipse.jgit.lib.ObjectIdSerialization.readNotNull;
+import static org.eclipse.jgit.lib.ObjectIdSerialization.writeCanBeNull;
+import static org.eclipse.jgit.lib.ObjectIdSerialization.writeNotNull;
+
+import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
+
+import org.eclipse.jgit.lib.ObjectId;
+
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.Objects;
+
+public class DiffSummaryKey implements Serializable {
+  public static final long serialVersionUID = 1L;
+
+  /** see PatchListKey#oldId */
+  private transient ObjectId oldId;
+
+  /** see PatchListKey#parentNum */
+  private transient Integer parentNum;
+
+  private transient ObjectId newId;
+  private transient Whitespace whitespace;
+
+  public static DiffSummaryKey fromPatchListKey(PatchListKey plk) {
+    return new DiffSummaryKey(plk.getOldId(), plk.getParentNum(),
+        plk.getNewId(), plk.getWhitespace());
+  }
+
+  private DiffSummaryKey(ObjectId oldId, Integer parentNum, ObjectId newId,
+      Whitespace whitespace) {
+    this.oldId = oldId;
+    this.parentNum = parentNum;
+    this.newId = newId;
+    this.whitespace = whitespace;
+  }
+
+  PatchListKey toPatchListKey() {
+    return new PatchListKey(oldId, parentNum, newId, whitespace);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(oldId, parentNum, newId, whitespace);
+  }
+
+  @Override
+  public boolean equals(final Object o) {
+    if (o instanceof DiffSummaryKey) {
+      DiffSummaryKey k = (DiffSummaryKey) o;
+      return Objects.equals(oldId, k.oldId)
+          && Objects.equals(parentNum, k.parentNum)
+          && Objects.equals(newId, k.newId)
+          && whitespace == k.whitespace;
+    }
+    return false;
+  }
+
+  @Override
+  public String toString() {
+    StringBuilder n = new StringBuilder();
+    n.append("DiffSummaryKey[");
+    n.append(oldId != null ? oldId.name() : "BASE");
+    n.append("..");
+    n.append(newId.name());
+    n.append(" ");
+    if (parentNum != null) {
+      n.append(parentNum);
+      n.append(" ");
+    }
+    n.append(whitespace.name());
+    n.append("]");
+    return n.toString();
+  }
+
+  private void writeObject(final ObjectOutputStream out) throws IOException {
+    writeCanBeNull(out, oldId);
+    out.writeInt(parentNum == null ? 0 : parentNum);
+    writeNotNull(out, newId);
+    Character c = PatchListKey.WHITESPACE_TYPES.get(whitespace);
+    if (c == null) {
+      throw new IOException("Invalid whitespace type: " + whitespace);
+    }
+    out.writeChar(c);
+  }
+
+  private void readObject(final ObjectInputStream in) throws IOException {
+    oldId = readCanBeNull(in);
+    int n = in.readInt();
+    parentNum = n == 0 ? null : Integer.valueOf(n);
+    newId = readNotNull(in);
+    char t = in.readChar();
+    whitespace = PatchListKey.WHITESPACE_TYPES.inverse().get(t);
+    if (whitespace == null) {
+      throw new IOException("Invalid whitespace type code: " + t);
+    }
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryLoader.java

@@ -27,20 +27,20 @@ import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.Callable;
 
-public class FileListLoader implements Callable<FileList> {
-  static final Logger log = LoggerFactory.getLogger(FileListLoader.class);
+public class DiffSummaryLoader implements Callable<DiffSummary> {
+  static final Logger log = LoggerFactory.getLogger(DiffSummaryLoader.class);
 
   public interface Factory {
-    FileListLoader create(PatchListKey key, Project.NameKey project);
+    DiffSummaryLoader create(DiffSummaryKey key, Project.NameKey project);
   }
 
   private final PatchListCache patchListCache;
-  private final PatchListKey key;
+  private final DiffSummaryKey key;
   private final Project.NameKey project;
 
   @AssistedInject
-  FileListLoader(PatchListCache plc,
-      @Assisted PatchListKey k,
+  DiffSummaryLoader(PatchListCache plc,
+      @Assisted DiffSummaryKey k,
       @Assisted Project.NameKey p) {
     patchListCache = plc;
     key = k;
@@ -48,12 +48,12 @@ public class FileListLoader implements Callable<FileList> {
   }
 
   @Override
-  public FileList call() throws Exception {
-    PatchList patchList = patchListCache.get(key, project);
-    return toFileList(patchList);
+  public DiffSummary call() throws Exception {
+    PatchList patchList = patchListCache.get(key.toPatchListKey(), project);
+    return toDiffSummary(patchList);
   }
 
-  static FileList toFileList(PatchList patchList) {
+  static DiffSummary toDiffSummary(PatchList patchList) {
     List<String> r = new ArrayList<>(patchList.getPatches().size());
     for (PatchListEntry e : patchList.getPatches()) {
       if (Patch.isMagic(e.getNewName())) {
@@ -75,6 +75,6 @@ public class FileListLoader implements Callable<FileList> {
       }
     }
     Collections.sort(r);
-    return new FileList(r.toArray(new String[r.size()]));
+    return new DiffSummary(r.toArray(new String[r.size()]));
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryWeigher.java

@@ -16,20 +16,19 @@ package com.google.gerrit.server.patch;
 
 import com.google.common.cache.Weigher;
 
-/** Computes memory usage for FileList in bytes of memory used */
-public class FileListWeigher implements Weigher<PatchListKey, FileList> {
+/** Computes memory usage for {@link DiffSummary} in bytes of memory used. */
+public class DiffSummaryWeigher implements
+    Weigher<DiffSummaryKey, DiffSummary> {
 
   @Override
-  public int weigh(PatchListKey key, FileList value) {
-    int size = 16 + 4 * 8 + 2 * 36; // Size of PatchListKey, 64 bit JVM
-
-    // Size of the list of paths ...
+  public int weigh(DiffSummaryKey key, DiffSummary value) {
+    int size = 16 + 4 * 8 + 2 * 36 // Size of DiffSummaryKey, 64 bit JVM
+        + 16 + 8 // Size of DiffSummary
+        + 16 + 8; // String[]
     for (String p : value.getPaths()) {
-      size += p.length();
+      size += 16 + 8 + 4 * 4 // String
+          + 16 + 8 + p.length() * 2; // char[]
     }
-    // ... plus new-line separators between paths
-    size += value.getPaths().size() - 1;
-
     return size;
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java

@@ -34,6 +34,6 @@ public interface PatchListCache {
   IntraLineDiff getIntraLineDiff(IntraLineDiffKey key,
       IntraLineDiffArgs args);
 
-  FileList getFileList(Change change, PatchSet patchSet)
+  DiffSummary getDiffSummary(Change change, PatchSet patchSet)
       throws PatchListNotAvailableException;
 }

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java

@@ -15,7 +15,7 @@
 
 package com.google.gerrit.server.patch;
 
-import static com.google.gerrit.server.patch.FileListLoader.toFileList;
+import static com.google.gerrit.server.patch.DiffSummaryLoader.toDiffSummary;
 
 import com.google.common.cache.Cache;
 import com.google.common.util.concurrent.UncheckedExecutionException;
@@ -41,7 +41,7 @@ import java.util.concurrent.ExecutionException;
 public class PatchListCacheImpl implements PatchListCache {
   static final String FILE_NAME = "diff";
   static final String INTRA_NAME = "diff_intraline";
-  static final String FILE_LIST = "diff_file_list";
+  static final String DIFF_SUMMARY = "diff_summary";
 
   public static Module module() {
     return new CacheModule() {
@@ -57,10 +57,10 @@ public class PatchListCacheImpl implements PatchListCache {
             .maximumWeight(10 << 20)
             .weigher(IntraLineWeigher.class);
 
-        factory(FileListLoader.Factory.class);
-        persist(FILE_LIST, PatchListKey.class, FileList.class)
+        factory(DiffSummaryLoader.Factory.class);
+        persist(DIFF_SUMMARY, DiffSummaryKey.class, DiffSummary.class)
             .maximumWeight(10 << 20)
-            .weigher(FileListWeigher.class)
+            .weigher(DiffSummaryWeigher.class)
             .diskLimit(1 << 30);
 
         bind(PatchListCacheImpl.class);
@@ -71,27 +71,27 @@ public class PatchListCacheImpl implements PatchListCache {
 
   private final Cache<PatchListKey, PatchList> fileCache;
   private final Cache<IntraLineDiffKey, IntraLineDiff> intraCache;
-  private final Cache<PatchListKey, FileList> fileListCache;
+  private final Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
   private final PatchListLoader.Factory fileLoaderFactory;
   private final IntraLineLoader.Factory intraLoaderFactory;
-  private final FileListLoader.Factory fileListLoaderFactory;
+  private final DiffSummaryLoader.Factory diffSummaryLoaderFactory;
   private final boolean computeIntraline;
 
   @Inject
   PatchListCacheImpl(
       @Named(FILE_NAME) Cache<PatchListKey, PatchList> fileCache,
       @Named(INTRA_NAME) Cache<IntraLineDiffKey, IntraLineDiff> intraCache,
-      @Named(FILE_LIST) Cache<PatchListKey, FileList> fileListCache,
+      @Named(DIFF_SUMMARY) Cache<DiffSummaryKey, DiffSummary> diffSummaryCache,
       PatchListLoader.Factory fileLoaderFactory,
       IntraLineLoader.Factory intraLoaderFactory,
-      FileListLoader.Factory fileListLoaderFactory,
+      DiffSummaryLoader.Factory diffSummaryLoaderFactory,
       @GerritServerConfig Config cfg) {
     this.fileCache = fileCache;
     this.intraCache = intraCache;
-    this.fileListCache = fileListCache;
+    this.diffSummaryCache = diffSummaryCache;
     this.fileLoaderFactory = fileLoaderFactory;
     this.intraLoaderFactory = intraLoaderFactory;
-    this.fileListLoaderFactory = fileListLoaderFactory;
+    this.diffSummaryLoaderFactory = diffSummaryLoaderFactory;
 
     this.computeIntraline =
         cfg.getBoolean("cache", INTRA_NAME, "enabled",
@@ -103,7 +103,9 @@ public class PatchListCacheImpl implements PatchListCache {
       throws PatchListNotAvailableException {
     try {
       PatchList pl = fileCache.get(key, fileLoaderFactory.create(key, project));
-      fileListCache.put(key, toFileList(pl));
+      diffSummaryCache.put(
+          DiffSummaryKey.fromPatchListKey(key),
+          toDiffSummary(pl));
       return pl;
     } catch (ExecutionException e) {
       PatchListLoader.log.warn("Error computing " + key, e);
@@ -159,19 +161,22 @@ public class PatchListCacheImpl implements PatchListCache {
   }
 
   @Override
-  public FileList getFileList(Change change, PatchSet patchSet)
+  public DiffSummary getDiffSummary(Change change, PatchSet patchSet)
       throws PatchListNotAvailableException {
     Project.NameKey project = change.getProject();
     ObjectId b = ObjectId.fromString(patchSet.getRevision().get());
     Whitespace ws = Whitespace.IGNORE_NONE;
-    return getFileList(PatchListKey.againstDefaultBase(b, ws), project);
+    return getDiffSummary(
+        DiffSummaryKey.fromPatchListKey(
+            PatchListKey.againstDefaultBase(b, ws)),
+        project);
   }
 
-  private FileList getFileList(PatchListKey key, Project.NameKey project)
-      throws PatchListNotAvailableException {
+  private DiffSummary getDiffSummary(DiffSummaryKey key,
+      Project.NameKey project) throws PatchListNotAvailableException {
     try {
-      return fileListCache.get(key,
-          fileListLoaderFactory.create(key, project));
+      return diffSummaryCache.get(key,
+          diffSummaryLoaderFactory.create(key, project));
     } catch (ExecutionException e) {
       PatchListLoader.log.warn("Error computing " + key, e);
       throw new PatchListNotAvailableException(e);
@@ -183,4 +188,4 @@ public class PatchListCacheImpl implements PatchListCache {
       throw e;
     }
   }
-}
+}

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java

@@ -92,6 +92,15 @@ public class PatchListKey implements Serializable {
     whitespace = ws;
   }
 
+  /** For use only by DiffSummaryKey. */
+  PatchListKey(ObjectId oldId, Integer parentNum, ObjectId newId,
+      Whitespace whitespace) {
+    this.oldId = oldId;
+    this.parentNum = parentNum;
+    this.newId = newId;
+    this.whitespace = whitespace;
+  }
+
   /** Old side commit, or null to assume ancestor or combined merge. */
   @Nullable
   public ObjectId getOldId() {

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -58,7 +58,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.patch.FileList;
+import com.google.gerrit.server.patch.DiffSummary;
 import com.google.gerrit.server.patch.PatchList;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
@@ -336,7 +336,7 @@ public class ChangeData {
   private List<PatchSetApproval> currentApprovals;
   private Map<Integer, List<String>> files;
   private Map<Integer, Optional<PatchList>> patchLists;
-  private Map<Integer, Optional<FileList>> fileLists;
+  private Map<Integer, Optional<DiffSummary>> diffSummaries;
   private Collection<Comment> publishedComments;
   private CurrentUser visibleTo;
   private ChangeControl changeControl;
@@ -588,7 +588,7 @@ public class ChangeData {
         return null;
       }
 
-      Optional<FileList> p = getFileList(c, ps);
+      Optional<DiffSummary> p = getDiffSummary(c, ps);
       if (!p.isPresent()) {
         List<String> emptyFileList = Collections.emptyList();
         if (lazyLoad) {
@@ -623,22 +623,22 @@ public class ChangeData {
     return r;
   }
 
-  private Optional<FileList> getFileList(Change c, PatchSet ps) {
+  private Optional<DiffSummary> getDiffSummary(Change c, PatchSet ps) {
     Integer psId = ps.getId().get();
-    if (fileLists == null) {
-      fileLists = new HashMap<>();
+    if (diffSummaries == null) {
+      diffSummaries = new HashMap<>();
     }
-    Optional<FileList> r = fileLists.get(psId);
+    Optional<DiffSummary> r = diffSummaries.get(psId);
     if (r == null) {
       if (!lazyLoad) {
         return Optional.absent();
       }
       try {
-        r = Optional.of(patchListCache.getFileList(c, ps));
+        r = Optional.of(patchListCache.getDiffSummary(c, ps));
       } catch (PatchListNotAvailableException e) {
         r = Optional.absent();
       }
-      fileLists.put(psId, r);
+      diffSummaries.put(psId, r);
     }
     return r;
   }