From 6976a30f443ece4815a977b0a5a897c0236018f7 Mon Sep 17 00:00:00 2001 From: Fabio Ponciroli Date: Thu, 3 Jun 2021 16:15:17 +0200 Subject: searchForReuse might impact performance in large repositories The search for reuse phase for *all* the objects scans *all* the packfiles, looking for the best candidate to serve back to the client. This can lead to an expensive operation when the number of packfiles and objects is high. Add parameter "pack.searchForReuseTimeout" to limit the time spent on this search. Change-Id: I54f5cddb6796fdc93ad9585c2ab4b44854fa6c48 --- .../eclipse/jgit/errors/SearchForReuseTimeout.java | 42 +++++++++++++++++++ .../src/org/eclipse/jgit/internal/JGitText.java | 1 + .../jgit/internal/storage/file/PackDirectory.java | 4 ++ .../jgit/internal/storage/pack/PackWriter.java | 44 ++++++++++++++++++++ .../src/org/eclipse/jgit/lib/ConfigConstants.java | 8 ++++ .../org/eclipse/jgit/storage/pack/PackConfig.java | 47 ++++++++++++++++++++++ .../src/org/eclipse/jgit/transport/UploadPack.java | 1 + 7 files changed, 147 insertions(+) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/errors/SearchForReuseTimeout.java (limited to 'org.eclipse.jgit/src/org') diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/SearchForReuseTimeout.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/SearchForReuseTimeout.java new file mode 100644 index 0000000000..402a850613 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/SearchForReuseTimeout.java @@ -0,0 +1,42 @@ +/* + * Copyright (C) 2021, Fabio Ponciroli + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.errors; + +import org.eclipse.jgit.internal.JGitText; + +import java.io.IOException; +import java.text.MessageFormat; +import java.time.Duration; + +/** + * Thrown when the search for reuse phase times out. + * + * @since 5.13 + */ +public class SearchForReuseTimeout extends IOException { + private static final long serialVersionUID = 1L; + + /** + * Construct a search for reuse timeout error. + * + * @param timeout + * time exceeded during the search for reuse phase. + */ + public SearchForReuseTimeout(Duration timeout) { + super(MessageFormat.format(JGitText.get().searchForReuseTimeout, + Long.valueOf(timeout.getSeconds()))); + } + + @Override + public synchronized Throwable fillInStackTrace() { + return this; + } +} \ No newline at end of file diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index fd54986f94..46d96df9c3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -660,6 +660,7 @@ public class JGitText extends TranslationBundle { /***/ public String saveFileStoreAttributesFailed; /***/ public String searchForReachableBranches; /***/ public String searchForReuse; + /***/ public String searchForReuseTimeout; /***/ public String searchForSizes; /***/ public String secondsAgo; /***/ public String selectingCommits; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java index 73745d8c64..f32909f44d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java @@ -33,6 +33,7 @@ import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.PackInvalidException; import org.eclipse.jgit.errors.PackMismatchException; +import org.eclipse.jgit.errors.SearchForReuseTimeout; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.pack.ObjectToPack; import org.eclipse.jgit.internal.storage.pack.PackExt; @@ -264,7 +265,10 @@ class PackDirectory { p.resetTransientErrorCount(); if (rep != null) { packer.select(otp, rep); + packer.checkSearchForReuseTimeout(); } + } catch (SearchForReuseTimeout e) { + break SEARCH; } catch (PackMismatchException e) { // Pack was modified; refresh the entire pack list. // diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java index 3e4b5df6aa..61f92d2e16 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java @@ -25,6 +25,7 @@ import java.io.OutputStream; import java.lang.ref.WeakReference; import java.security.MessageDigest; import java.text.MessageFormat; +import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -54,6 +55,7 @@ import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.LargeObjectException; import org.eclipse.jgit.errors.MissingObjectException; +import org.eclipse.jgit.errors.SearchForReuseTimeout; import org.eclipse.jgit.errors.StoredObjectRepresentationNotAvailableException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.file.PackBitmapIndexBuilder; @@ -262,6 +264,12 @@ public class PackWriter implements AutoCloseable { private boolean indexDisabled; + private boolean checkSearchForReuseTimeout = false; + + private final Duration searchForReuseTimeout; + + private long searchForReuseStartTimeEpoc; + private int depth; private Collection unshallowObjects; @@ -356,6 +364,7 @@ public class PackWriter implements AutoCloseable { deltaBaseAsOffset = config.isDeltaBaseAsOffset(); reuseDeltas = config.isReuseDeltas(); + searchForReuseTimeout = config.getSearchForReuseTimeout(); reuseValidate = true; // be paranoid by default stats = statsAccumulator != null ? statsAccumulator : new PackStatistics.Accumulator(); @@ -404,6 +413,24 @@ public class PackWriter implements AutoCloseable { return deltaBaseAsOffset; } + /** + * Check whether the search for reuse phase is taking too long. This could + * be the case when the number of objects and pack files is high and the + * system is under pressure. If that's the case and + * checkSearchForReuseTimeout is true abort the search. + * + * @throws SearchForReuseTimeout + * if the search for reuse is taking too long. + */ + public void checkSearchForReuseTimeout() throws SearchForReuseTimeout { + if (checkSearchForReuseTimeout + && Duration.ofMillis(System.currentTimeMillis() + - searchForReuseStartTimeEpoc) + .compareTo(searchForReuseTimeout) > 0) { + throw new SearchForReuseTimeout(searchForReuseTimeout); + } + } + /** * Set writer delta base format. Delta base can be written as an offset in a * pack file (new approach reducing file size) or as an object id (legacy @@ -419,6 +446,22 @@ public class PackWriter implements AutoCloseable { this.deltaBaseAsOffset = deltaBaseAsOffset; } + /** + * Set the writer to check for long search for reuse, exceeding the timeout. + * Selecting an object representation can be an expensive operation. It is + * possible to set a max search for reuse time (see + * PackConfig#CONFIG_KEY_SEARCH_FOR_REUSE_TIMEOUT for more details). + * + * However some operations, i.e.: GC, need to find the best candidate + * regardless how much time the operation will need to finish. + * + * This method enables the search for reuse timeout check, otherwise + * disabled. + */ + public void enableSearchForReuseTimeout() { + this.checkSearchForReuseTimeout = true; + } + /** * Check if the writer will reuse commits that are already stored as deltas. * @@ -1306,6 +1349,7 @@ public class PackWriter implements AutoCloseable { cnt += objectsLists[OBJ_TAG].size(); long start = System.currentTimeMillis(); + searchForReuseStartTimeEpoc = start; beginPhase(PackingPhase.FINDING_SOURCES, monitor, cnt); if (cnt <= 4096) { // For small object counts, do everything as one list. diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java index 3e3d9b5694..b6f4798dce 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java @@ -736,4 +736,12 @@ public final class ConfigConstants { * @since 5.11 */ public static final String CONFIG_KEY_DEFAULT_BRANCH = "defaultbranch"; + + /** + * The "pack.searchForReuseTimeout" key + * + * @since 5.13 + */ + public static final String CONFIG_KEY_SEARCH_FOR_REUSE_TIMEOUT = "searchforreusetimeout"; + } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackConfig.java index f76dd2721f..6aa8be642d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackConfig.java @@ -29,6 +29,7 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_INDEXVERSION; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_MIN_SIZE_PREVENT_RACYPACK; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_REUSE_DELTAS; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_REUSE_OBJECTS; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_SEARCH_FOR_REUSE_TIMEOUT; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_SINGLE_PACK; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_THREADS; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_WAIT_PREVENT_RACYPACK; @@ -36,7 +37,9 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_WINDOW; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_WINDOW_MEMORY; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_PACK_SECTION; +import java.time.Duration; import java.util.concurrent.Executor; +import java.util.concurrent.TimeUnit; import java.util.zip.Deflater; import org.eclipse.jgit.internal.storage.file.PackIndexWriter; @@ -222,6 +225,16 @@ public class PackConfig { */ public static final int DEFAULT_BITMAP_INACTIVE_BRANCH_AGE_IN_DAYS = 90; + /** + * Default max time to spend during the search for reuse phase. This + * optimization is disabled by default: {@value} + * + * @see #setSearchForReuseTimeout(Duration) + * @since 5.13 + */ + public static final Duration DEFAULT_SEARCH_FOR_REUSE_TIMEOUT = Duration + .ofSeconds(Integer.MAX_VALUE); + private int compressionLevel = Deflater.DEFAULT_COMPRESSION; private boolean reuseDeltas = DEFAULT_REUSE_DELTAS; @@ -272,6 +285,8 @@ public class PackConfig { private int bitmapInactiveBranchAgeInDays = DEFAULT_BITMAP_INACTIVE_BRANCH_AGE_IN_DAYS; + private Duration searchForReuseTimeout = DEFAULT_SEARCH_FOR_REUSE_TIMEOUT; + private boolean cutDeltaChains; private boolean singlePack; @@ -342,6 +357,7 @@ public class PackConfig { this.bitmapInactiveBranchAgeInDays = cfg.bitmapInactiveBranchAgeInDays; this.cutDeltaChains = cfg.cutDeltaChains; this.singlePack = cfg.singlePack; + this.searchForReuseTimeout = cfg.searchForReuseTimeout; } /** @@ -1103,6 +1119,18 @@ public class PackConfig { return bitmapInactiveBranchAgeInDays; } + /** + * Get the max time to spend during the search for reuse phase. + * + * Default setting: {@value #DEFAULT_SEARCH_FOR_REUSE_TIMEOUT} + * + * @return the maximum time to spend during the search for reuse phase. + * @since 5.13 + */ + public Duration getSearchForReuseTimeout() { + return searchForReuseTimeout; + } + /** * Set the age in days that marks a branch as "inactive". * @@ -1116,6 +1144,19 @@ public class PackConfig { bitmapInactiveBranchAgeInDays = ageInDays; } + /** + * Set the max time to spend during the search for reuse phase. + * + * @param timeout + * max time allowed during the search for reuse phase + * + * Default setting: {@value #DEFAULT_SEARCH_FOR_REUSE_TIMEOUT} + * @since 5.13 + */ + public void setSearchForReuseTimeout(Duration timeout) { + searchForReuseTimeout = timeout; + } + /** * Update properties by setting fields from the configuration. * @@ -1179,6 +1220,10 @@ public class PackConfig { setBitmapInactiveBranchAgeInDays(rc.getInt(CONFIG_PACK_SECTION, CONFIG_KEY_BITMAP_INACTIVE_BRANCH_AGE_INDAYS, getBitmapInactiveBranchAgeInDays())); + setSearchForReuseTimeout(Duration.ofSeconds(rc.getTimeUnit( + CONFIG_PACK_SECTION, null, + CONFIG_KEY_SEARCH_FOR_REUSE_TIMEOUT, + getSearchForReuseTimeout().getSeconds(), TimeUnit.SECONDS))); setWaitPreventRacyPack(rc.getBoolean(CONFIG_PACK_SECTION, CONFIG_KEY_WAIT_PREVENT_RACYPACK, isWaitPreventRacyPack())); setMinSizePreventRacyPack(rc.getLong(CONFIG_PACK_SECTION, @@ -1216,6 +1261,8 @@ public class PackConfig { .append(getBitmapExcessiveBranchCount()); b.append(", bitmapInactiveBranchAge=") //$NON-NLS-1$ .append(getBitmapInactiveBranchAgeInDays()); + b.append(", searchForReuseTimeout") //$NON-NLS-1$ + .append(getSearchForReuseTimeout()); b.append(", singlePack=").append(getSinglePack()); //$NON-NLS-1$ return b.toString(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index ecf1751932..37a1c1e589 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -2359,6 +2359,7 @@ public class UploadPack { GitProtocolConstants.SECTION_PACKFILE + '\n'); } } + pw.enableSearchForReuseTimeout(); pw.writePack(pm, NullProgressMonitor.INSTANCE, packOut); if (msgOut != NullOutputStream.INSTANCE) { -- cgit v1.2.3