From 9c87603a8b552149fb8753f759c43804ae0050ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 22 Sep 2016 22:45:24 +0200 Subject: [PATCH 1/2] Support for setting diskLimit from CacheModule bindings For some persistent caches we may want a larger default diskLimit than the overall default of 128MiB. Change-Id: I9f8c85d7187d2edfe5ba48ea27c7b5f5e37e7abe --- .../gerrit/server/cache/h2/H2CacheFactory.java | 2 +- .../gerrit/server/cache/CacheBinding.java | 4 ++++ .../gerrit/server/cache/CacheProvider.java | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java b/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java index 8141dfb149..f7381a39c8 100644 --- a/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java +++ b/gerrit-cache-h2/src/main/java/com/google/gerrit/server/cache/h2/H2CacheFactory.java @@ -178,7 +178,7 @@ class H2CacheFactory implements PersistentCacheFactory, LifecycleListener { public LoadingCache build( CacheBinding def, CacheLoader loader) { - long limit = config.getLong("cache", def.name(), "diskLimit", 128 << 20); + long limit = config.getLong("cache", def.name(), "diskLimit", def.diskLimit()); if (cacheDir == null || limit <= 0) { return defaultFactory.build(def, loader); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheBinding.java b/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheBinding.java index 70628718de..343827cb83 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheBinding.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheBinding.java @@ -26,6 +26,9 @@ public interface CacheBinding { /** Set the total size of the cache. */ CacheBinding maximumWeight(long weight); + /** Set the total on-disk limit of the cache */ + CacheBinding diskLimit(long limit); + /** Set the time an element lives before being expired. */ CacheBinding expireAfterWrite(long duration, TimeUnit durationUnits); @@ -39,6 +42,7 @@ public interface CacheBinding { TypeLiteral keyType(); TypeLiteral valueType(); long maximumWeight(); + long diskLimit(); @Nullable Long expireAfterWrite(TimeUnit unit); @Nullable Weigher weigher(); @Nullable CacheLoader loader(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheProvider.java index 6d9ae0f34b..c73760c259 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/cache/CacheProvider.java @@ -38,6 +38,7 @@ class CacheProvider private final TypeLiteral valType; private boolean persist; private long maximumWeight; + private long diskLimit; private Long expireAfterWrite; private Provider> loader; private Provider> weigher; @@ -85,6 +86,15 @@ class CacheProvider return this; } + @Override + public CacheBinding diskLimit(long limit) { + Preconditions.checkState(!frozen, "binding frozen, cannot be modified"); + Preconditions.checkState(persist, + "diskLimit supported for persistent caches only"); + diskLimit = limit; + return this; + } + @Override public CacheBinding expireAfterWrite(long duration, TimeUnit unit) { Preconditions.checkState(!frozen, "binding frozen, cannot be modified"); @@ -129,6 +139,14 @@ class CacheProvider return maximumWeight; } + @Override + public long diskLimit() { + if (diskLimit > 0) { + return diskLimit; + } + return 128 << 20; + } + @Override @Nullable public Long expireAfterWrite(TimeUnit unit) { From e789f2edffdbb0aa18acee89e80321445066175a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 22 Sep 2016 10:37:34 +0200 Subject: [PATCH 2/2] Add new persistent cache "diff_file_path_list" to cache file paths only The new cache caches list of paths of changed files. Main intention of the new cache is to speed up change reindexing. By caching list of changed files we avoid computing diff for that purpose. NOTE: The existing "diff" cache implicitly caches list of changed files but its entries are quite large to keep it populated for all changes. Change-Id: I45f90deeba9fe70f32e4b0a5657bfb211ecc2eeb --- Documentation/config-gerrit.txt | 16 +++- .../google/gerrit/server/patch/FileList.java | 65 +++++++++++++++ .../gerrit/server/patch/FileListLoader.java | 80 +++++++++++++++++++ .../gerrit/server/patch/FileListWeigher.java | 35 ++++++++ .../gerrit/server/patch/PatchListCache.java | 3 + .../server/patch/PatchListCacheImpl.java | 46 ++++++++++- .../server/query/change/ChangeData.java | 49 ++++++------ 7 files changed, 266 insertions(+), 28 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/FileList.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListLoader.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListWeigher.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 8661d408d6..7d029090d4 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -611,6 +611,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) * `"plugin_resources"`: default is 2m (2 MiB of memory) + @@ -626,7 +627,10 @@ accessed order until the cache fits within this limit. Caches may grow larger than this during the day, as the size check is only performed once every 24 hours. + -Default is 128 MiB per cache. +Default is 128 MiB per cache, except: ++ +* `"diff_file_list"`: default is `1g` (1 GiB of disk space) + + If 0, disk storage for the cache is disabled. @@ -698,6 +702,16 @@ 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"`:: ++ +Each item caches list of file paths which are different between two +commits. Gerrit uses this cache to accelerate computing of the list +of paths of changed files. ++ +Ideally, disk limit of this cache is large enough to cover all changes. +This should significantly speed up change reindexing, especially +full offline reindexing. + cache `"git_tags"`:: + If branch or reference level READ access controls are used, this 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/FileList.java new file mode 100644 index 0000000000..7715f7d789 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileList.java @@ -0,0 +1,65 @@ +// 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 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.writeString; + +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; +import java.io.Serializable; +import java.util.Arrays; +import java.util.Collections; +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; + + private transient String[] paths; + + public FileList(String[] paths) { + this.paths = paths; + } + + public List getPaths() { + return Collections.unmodifiableList(Arrays.asList(paths)); + } + + private void writeObject(ObjectOutputStream output) throws IOException { + ByteArrayOutputStream buf = new ByteArrayOutputStream(); + try (DeflaterOutputStream out = new DeflaterOutputStream(buf)) { + writeString(out, Joiner.on('\n').join(paths)); + } + 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()]); + } + } +} 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/FileListLoader.java new file mode 100644 index 0000000000..f73c139187 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListLoader.java @@ -0,0 +1,80 @@ +// 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 com.google.gerrit.reviewdb.client.Patch; +import com.google.gerrit.reviewdb.client.Project; +import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.ArrayList; +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 interface Factory { + FileListLoader create(PatchListKey key, Project.NameKey project); + } + + private final PatchListCache patchListCache; + private final PatchListKey key; + private final Project.NameKey project; + + @AssistedInject + FileListLoader(PatchListCache plc, + @Assisted PatchListKey k, + @Assisted Project.NameKey p) { + patchListCache = plc; + key = k; + project = p; + } + + @Override + public FileList call() throws Exception { + PatchList patchList = patchListCache.get(key, project); + return toFileList(patchList); + } + + static FileList toFileList(PatchList patchList) { + List r = new ArrayList<>(patchList.getPatches().size()); + for (PatchListEntry e : patchList.getPatches()) { + if (Patch.isMagic(e.getNewName())) { + continue; + } + switch (e.getChangeType()) { + case ADDED: + case MODIFIED: + case DELETED: + case COPIED: + case REWRITE: + r.add(e.getNewName()); + break; + + case RENAMED: + r.add(e.getOldName()); + r.add(e.getNewName()); + break; + } + } + Collections.sort(r); + return new FileList(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/FileListWeigher.java new file mode 100644 index 0000000000..7e884f7702 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/FileListWeigher.java @@ -0,0 +1,35 @@ +// 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 com.google.common.cache.Weigher; + +/** Computes memory usage for FileList in bytes of memory used */ +public class FileListWeigher 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 ... + for (String p : value.getPaths()) { + size += p.length(); + } + // ... 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 8a2403f40f..7b413f663f 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 @@ -33,4 +33,7 @@ public interface PatchListCache { IntraLineDiff getIntraLineDiff(IntraLineDiffKey key, IntraLineDiffArgs args); + + FileList getFileList(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 abafad798a..3e96ab513d 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,6 +15,7 @@ package com.google.gerrit.server.patch; +import static com.google.gerrit.server.patch.FileListLoader.toFileList; import com.google.common.cache.Cache; import com.google.common.util.concurrent.UncheckedExecutionException; import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; @@ -39,6 +40,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"; public static Module module() { return new CacheModule() { @@ -54,6 +56,12 @@ public class PatchListCacheImpl implements PatchListCache { .maximumWeight(10 << 20) .weigher(IntraLineWeigher.class); + factory(FileListLoader.Factory.class); + persist(FILE_LIST, PatchListKey.class, FileList.class) + .maximumWeight(10 << 20) + .weigher(FileListWeigher.class) + .diskLimit(1 << 30); + bind(PatchListCacheImpl.class); bind(PatchListCache.class).to(PatchListCacheImpl.class); } @@ -62,21 +70,27 @@ public class PatchListCacheImpl implements PatchListCache { private final Cache fileCache; private final Cache intraCache; + private final Cache fileListCache; private final PatchListLoader.Factory fileLoaderFactory; private final IntraLineLoader.Factory intraLoaderFactory; + private final FileListLoader.Factory fileListLoaderFactory; private final boolean computeIntraline; @Inject PatchListCacheImpl( @Named(FILE_NAME) Cache fileCache, @Named(INTRA_NAME) Cache intraCache, + @Named(FILE_LIST) Cache fileListCache, PatchListLoader.Factory fileLoaderFactory, IntraLineLoader.Factory intraLoaderFactory, + FileListLoader.Factory fileListLoaderFactory, @GerritServerConfig Config cfg) { this.fileCache = fileCache; this.intraCache = intraCache; + this.fileListCache = fileListCache; this.fileLoaderFactory = fileLoaderFactory; this.intraLoaderFactory = intraLoaderFactory; + this.fileListLoaderFactory = fileListLoaderFactory; this.computeIntraline = cfg.getBoolean("cache", INTRA_NAME, "enabled", @@ -87,7 +101,9 @@ public class PatchListCacheImpl implements PatchListCache { public PatchList get(PatchListKey key, Project.NameKey project) throws PatchListNotAvailableException { try { - return fileCache.get(key, fileLoaderFactory.create(key, project)); + PatchList pl = fileCache.get(key, fileLoaderFactory.create(key, project)); + fileListCache.put(key, toFileList(pl)); + return pl; } catch (ExecutionException e) { PatchListLoader.log.warn("Error computing " + key, e); throw new PatchListNotAvailableException(e); @@ -140,4 +156,30 @@ public class PatchListCacheImpl implements PatchListCache { } return new IntraLineDiff(IntraLineDiff.Status.DISABLED); } -} + + @Override + public FileList getFileList(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); + } + + private FileList getFileList(PatchListKey key, Project.NameKey project) + throws PatchListNotAvailableException { + try { + return fileListCache.get(key, + fileListLoaderFactory.create(key, project)); + } catch (ExecutionException e) { + PatchListLoader.log.warn("Error computing " + key, e); + throw new PatchListNotAvailableException(e); + } catch (UncheckedExecutionException e) { + if (e.getCause() instanceof LargeObjectException) { + PatchListLoader.log.warn("Error computing " + key, e); + throw new PatchListNotAvailableException(e); + } + throw e; + } + } +} \ No newline at end of file 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 c7c71a7827..ab98dabf89 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 @@ -38,7 +38,6 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Comment; -import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; @@ -59,9 +58,9 @@ 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.PatchList; import com.google.gerrit.server.patch.PatchListCache; -import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; @@ -337,6 +336,7 @@ public class ChangeData { private List currentApprovals; private Map> files; private Map> patchLists; + private Map> fileLists; private Collection publishedComments; private CurrentUser visibleTo; private ChangeControl changeControl; @@ -589,7 +589,7 @@ public class ChangeData { return null; } - Optional p = getPatchList(c, ps); + Optional p = getFileList(c, ps); if (!p.isPresent()) { List emptyFileList = Collections.emptyList(); if (lazyLoad) { @@ -598,28 +598,7 @@ public class ChangeData { return emptyFileList; } - r = new ArrayList<>(p.get().getPatches().size()); - for (PatchListEntry e : p.get().getPatches()) { - if (Patch.isMagic(e.getNewName())) { - continue; - } - switch (e.getChangeType()) { - case ADDED: - case MODIFIED: - case DELETED: - case COPIED: - case REWRITE: - r.add(e.getNewName()); - break; - - case RENAMED: - r.add(e.getOldName()); - r.add(e.getNewName()); - break; - } - } - Collections.sort(r); - r = Collections.unmodifiableList(r); + r = p.get().getPaths(); files.put(psId, r); } return r; @@ -645,6 +624,26 @@ public class ChangeData { return r; } + private Optional getFileList(Change c, PatchSet ps) { + Integer psId = ps.getId().get(); + if (fileLists == null) { + fileLists = new HashMap<>(); + } + Optional r = fileLists.get(psId); + if (r == null) { + if (!lazyLoad) { + return Optional.absent(); + } + try { + r = Optional.of(patchListCache.getFileList(c, ps)); + } catch (PatchListNotAvailableException e) { + r = Optional.absent(); + } + fileLists.put(psId, r); + } + return r; + } + private Optional computeChangedLines() throws OrmException { Change c = change(); if (c == null) {