diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 96472949e8..004d32b8d1 100644 --- a/Documentation/config-gerrit.txt +++ b/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 diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileList.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummary.java similarity index 65% rename from gerrit-server/src/main/java/com/google/gerrit/server/patch/FileList.java rename to gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummary.java index 7715f7d789..ae4589f847 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileList.java +++ b/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 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); + } } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryKey.java new file mode 100644 index 0000000000..4c708c4467 --- /dev/null +++ b/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); + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryLoader.java similarity index 74% rename from gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListLoader.java rename to gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryLoader.java index f73c139187..43e2392571 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListLoader.java +++ b/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 { - static final Logger log = LoggerFactory.getLogger(FileListLoader.class); +public class DiffSummaryLoader implements Callable { + 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 { } @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 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 { } } Collections.sort(r); - return new FileList(r.toArray(new String[r.size()])); + return new DiffSummary(r.toArray(new String[r.size()])); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListWeigher.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryWeigher.java similarity index 63% rename from gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListWeigher.java rename to gerrit-server/src/main/java/com/google/gerrit/server/patch/DiffSummaryWeigher.java index 7e884f7702..548f999827 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListWeigher.java +++ b/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 { +/** Computes memory usage for {@link DiffSummary} in bytes of memory used. */ +public class DiffSummaryWeigher implements + Weigher { @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; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java index 7b413f663f..848b78ff0b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCache.java +++ b/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; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java index e367bbef07..f1490f6fc1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java +++ b/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 fileCache; private final Cache intraCache; - private final Cache fileListCache; + private final Cache 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 fileCache, @Named(INTRA_NAME) Cache intraCache, - @Named(FILE_LIST) Cache fileListCache, + @Named(DIFF_SUMMARY) Cache 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; } } -} \ No newline at end of file +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java index 22f7bf31b6..6bb32a243a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListKey.java +++ b/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() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index d3c8b1e078..407d1137c5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/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 currentApprovals; private Map> files; private Map> patchLists; - private Map> fileLists; + private Map> diffSummaries; private Collection publishedComments; private CurrentUser visibleTo; private ChangeControl changeControl; @@ -588,7 +588,7 @@ public class ChangeData { return null; } - Optional p = getFileList(c, ps); + Optional p = getDiffSummary(c, ps); if (!p.isPresent()) { List emptyFileList = Collections.emptyList(); if (lazyLoad) { @@ -623,22 +623,22 @@ public class ChangeData { return r; } - private Optional getFileList(Change c, PatchSet ps) { + private Optional getDiffSummary(Change c, PatchSet ps) { Integer psId = ps.getId().get(); - if (fileLists == null) { - fileLists = new HashMap<>(); + if (diffSummaries == null) { + diffSummaries = new HashMap<>(); } - Optional r = fileLists.get(psId); + Optional 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; }