diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackDescriptionTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackDescriptionTest.java new file mode 100644 index 000000000..6029e9cae --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/dfs/DfsPackDescriptionTest.java @@ -0,0 +1,273 @@ +/* + * Copyright (C) 2018, Google LLC. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.internal.storage.dfs; + +import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.COMPACT; +import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.GC; +import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.GC_REST; +import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.INSERT; +import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.RECEIVE; +import static org.eclipse.jgit.internal.storage.pack.PackExt.INDEX; +import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; +import static org.junit.Assert.assertEquals; + +import java.util.Comparator; +import java.util.concurrent.atomic.AtomicInteger; + +import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource; +import org.junit.Before; +import org.junit.Test; + +public final class DfsPackDescriptionTest { + private AtomicInteger counter; + + @Before + public void setUp() { + counter = new AtomicInteger(); + } + + @Test + public void objectLookupComparatorEqual() throws Exception { + DfsPackDescription a = create(RECEIVE); + a.setFileSize(PACK, 1); + a.setFileSize(INDEX, 1); + a.setLastModified(1); + a.setObjectCount(1); + a.setMaxUpdateIndex(1); + + DfsPackDescription b = create(INSERT); + b.setFileSize(PACK, 1); + b.setFileSize(INDEX, 2); + b.setLastModified(1); + b.setObjectCount(1); + b.setMaxUpdateIndex(2); + + assertComparesEqual(DfsPackDescription.objectLookupComparator(), a, b); + } + + @Test + public void objectLookupComparatorPackSource() throws Exception { + DfsPackDescription a = create(COMPACT); + a.setFileSize(PACK, 2); + a.setLastModified(1); + a.setObjectCount(2); + + DfsPackDescription b = create(GC); + b.setFileSize(PACK, 1); + b.setLastModified(2); + b.setObjectCount(1); + + assertComparesLessThan(DfsPackDescription.objectLookupComparator(), a, b); + } + + @Test + public void objectLookupComparatorGcFileSize() throws Exception { + // a is older and smaller. + DfsPackDescription a = create(GC_REST); + a.setFileSize(PACK, 100); + a.setLastModified(1); + a.setObjectCount(2); + + // b is newer and larger. + DfsPackDescription b = create(GC_REST); + b.setFileSize(PACK, 200); + b.setLastModified(2); + b.setObjectCount(1); + + // Since they have the same GC type, tiebreaker is size, and a comes first. + assertComparesLessThan(DfsPackDescription.objectLookupComparator(), a, b); + } + + @Test + public void objectLookupComparatorNonGcLastModified() + throws Exception { + // a is older and smaller. + DfsPackDescription a = create(INSERT); + a.setFileSize(PACK, 100); + a.setLastModified(1); + a.setObjectCount(2); + + // b is newer and larger. + DfsPackDescription b = create(INSERT); + b.setFileSize(PACK, 200); + b.setLastModified(2); + b.setObjectCount(1); + + // Since they have the same type but not GC, tiebreaker is last modified, + // and b comes first. + assertComparesLessThan(DfsPackDescription.objectLookupComparator(), b, a); + } + + @Test + public void objectLookupComparatorObjectCount() throws Exception { + DfsPackDescription a = create(INSERT); + a.setObjectCount(1); + + DfsPackDescription b = create(INSERT); + b.setObjectCount(2); + + assertComparesLessThan(DfsPackDescription.objectLookupComparator(), a, b); + } + + @Test + public void reftableComparatorEqual() throws Exception { + DfsPackDescription a = create(INSERT); + a.setFileSize(PACK, 100); + a.setObjectCount(1); + + DfsPackDescription b = create(INSERT); + b.setFileSize(PACK, 200); + a.setObjectCount(2); + + assertComparesEqual(DfsPackDescription.reftableComparator(), a, b); + } + + @Test + public void reftableComparatorPackSource() throws Exception { + DfsPackDescription a = create(INSERT); + a.setMaxUpdateIndex(1); + a.setLastModified(1); + + DfsPackDescription b = create(GC); + b.setMaxUpdateIndex(2); + b.setLastModified(2); + + assertComparesLessThan(DfsPackDescription.reftableComparator(), b, a); + } + + @Test + public void reftableComparatorMaxUpdateIndex() throws Exception { + DfsPackDescription a = create(INSERT); + a.setMaxUpdateIndex(1); + a.setLastModified(2); + + DfsPackDescription b = create(INSERT); + b.setMaxUpdateIndex(2); + b.setLastModified(1); + + assertComparesLessThan(DfsPackDescription.reftableComparator(), a, b); + } + + @Test + public void reftableComparatorLastModified() throws Exception { + DfsPackDescription a = create(INSERT); + a.setLastModified(1); + + DfsPackDescription b = create(INSERT); + b.setLastModified(2); + + assertComparesLessThan(DfsPackDescription.reftableComparator(), a, b); + } + + @Test + public void reuseComparatorEqual() throws Exception { + DfsPackDescription a = create(RECEIVE); + a.setFileSize(PACK, 1); + a.setFileSize(INDEX, 1); + a.setLastModified(1); + a.setObjectCount(1); + a.setMaxUpdateIndex(1); + + DfsPackDescription b = create(INSERT); + b.setFileSize(PACK, 2); + b.setFileSize(INDEX, 2); + b.setLastModified(2); + b.setObjectCount(2); + b.setMaxUpdateIndex(2); + + assertComparesEqual(DfsPackDescription.reuseComparator(), a, b); + } + + @Test + public void reuseComparatorGcPackSize() throws Exception { + DfsPackDescription a = create(GC_REST); + a.setFileSize(PACK, 1); + a.setFileSize(INDEX, 1); + a.setLastModified(2); + a.setObjectCount(1); + a.setMaxUpdateIndex(1); + + DfsPackDescription b = create(GC_REST); + b.setFileSize(PACK, 2); + b.setFileSize(INDEX, 2); + b.setLastModified(1); + b.setObjectCount(2); + b.setMaxUpdateIndex(2); + + assertComparesLessThan(DfsPackDescription.reuseComparator(), b, a); + } + + private DfsPackDescription create(PackSource source) { + return new DfsPackDescription( + new DfsRepositoryDescription("repo"), + "pack_" + counter.incrementAndGet(), + source); + } + + private static void assertComparesEqual( + Comparator comparator, T o1, T o2) { + assertEquals( + "first object must compare equal to itself", + 0, comparator.compare(o1, o1)); + assertEquals( + "second object must compare equal to itself", + 0, comparator.compare(o2, o2)); + assertEquals( + "first object must compare equal to second object", + 0, comparator.compare(o1, o2)); + } + + private static void assertComparesLessThan( + Comparator comparator, T o1, T o2) { + assertEquals( + "first object must compare equal to itself", + 0, comparator.compare(o1, o1)); + assertEquals( + "second object must compare equal to itself", + 0, comparator.compare(o2, o2)); + assertEquals( + "first object must compare less than second object", + -1, comparator.compare(o1, o2)); + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java index e58dd0159..ce5efd0a8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java @@ -607,7 +607,7 @@ public abstract class DfsObjDatabase extends ObjectDatabase { Map reftables = reftableMap(old); List scanned = listPacks(); - Collections.sort(scanned); + Collections.sort(scanned, DfsPackDescription.objectLookupComparator()); List newPacks = new ArrayList<>(scanned.size()); List newReftables = new ArrayList<>(scanned.size()); @@ -668,26 +668,9 @@ public abstract class DfsObjDatabase extends ObjectDatabase { * @return comparator to sort {@link DfsReftable} by priority. */ protected Comparator reftableComparator() { - return (fa, fb) -> { - DfsPackDescription a = fa.getPackDescription(); - DfsPackDescription b = fb.getPackDescription(); - - // GC, COMPACT reftables first by reversing default order. - int c = PackSource.DEFAULT_COMPARATOR.reversed() - .compare(a.getPackSource(), b.getPackSource()); - if (c != 0) { - return c; - } - - // Lower maxUpdateIndex first. - c = Long.signum(a.getMaxUpdateIndex() - b.getMaxUpdateIndex()); - if (c != 0) { - return c; - } - - // Older reftable first. - return Long.signum(a.getLastModified() - b.getLastModified()); - }; + return Comparator.comparing( + DfsReftable::getPackDescription, + DfsPackDescription.reftableComparator()); } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java index b43b9b178..127ee6bf1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackCompactor.java @@ -404,12 +404,11 @@ public class DfsPackCompactor { // Sort packs by description ordering, this places newer packs before // older packs, allowing the PackWriter to be handed newer objects // first and older objects last. - Collections.sort(srcPacks, new Comparator() { - @Override - public int compare(DfsPackFile a, DfsPackFile b) { - return a.getPackDescription().compareTo(b.getPackDescription()); - } - }); + Collections.sort( + srcPacks, + Comparator.comparing( + DfsPackFile::getPackDescription, + DfsPackDescription.objectLookupComparator())); rw = new RevWalk(ctx); added = rw.newFlag("ADDED"); //$NON-NLS-1$ diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java index 2e7f6a18f..53140baf4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java @@ -47,6 +47,7 @@ import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; import static org.eclipse.jgit.internal.storage.pack.PackExt.REFTABLE; import java.util.Arrays; +import java.util.Comparator; import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource; @@ -62,7 +63,88 @@ import org.eclipse.jgit.storage.pack.PackStatistics; * Instances of this class are cached with the DfsPackFile, and should not be * modified once initialized and presented to the JGit DFS library. */ -public class DfsPackDescription implements Comparable { +public class DfsPackDescription { + /** + * Comparator for packs when looking up objects in indexes. + *

+ * This comparator tries to position packs in the order readers should examine + * them when looking for objects by SHA-1. The default tries to sort packs + * with more recent modification dates before older packs, and packs with + * fewer objects before packs with more objects. + *

+ * Uses {@link PackSource#DEFAULT_COMPARATOR} for sorting sources. + * + * @return comparator. + */ + public static Comparator objectLookupComparator() { + return Comparator.comparing( + DfsPackDescription::getPackSource, PackSource.DEFAULT_COMPARATOR) + .thenComparing((a, b) -> { + PackSource as = a.getPackSource(); + PackSource bs = b.getPackSource(); + + // Tie break GC type packs by smallest first. There should be at most + // one of each source, but when multiple exist concurrent GCs may have + // run. Preferring the smaller file selects higher quality delta + // compression, placing less demand on the DfsBlockCache. + if (as == bs && isGC(as)) { + int cmp = Long.signum(a.getFileSize(PACK) - b.getFileSize(PACK)); + if (cmp != 0) { + return cmp; + } + } + + // Newer packs should sort first. + int cmp = Long.signum(b.getLastModified() - a.getLastModified()); + if (cmp != 0) { + return cmp; + } + + // Break ties on smaller index. Readers may get lucky and find + // the object they care about in the smaller index. This also pushes + // big historical packs to the end of the list, due to more objects. + return Long.signum(a.getObjectCount() - b.getObjectCount()); + }); + } + + static Comparator reftableComparator() { + return (a, b) -> { + // GC, COMPACT reftables first by reversing default order. + int c = PackSource.DEFAULT_COMPARATOR.reversed() + .compare(a.getPackSource(), b.getPackSource()); + if (c != 0) { + return c; + } + + // Lower maxUpdateIndex first. + c = Long.signum(a.getMaxUpdateIndex() - b.getMaxUpdateIndex()); + if (c != 0) { + return c; + } + + // Older reftable first. + return Long.signum(a.getLastModified() - b.getLastModified()); + }; + } + + static Comparator reuseComparator() { + return (a, b) -> { + PackSource as = a.getPackSource(); + PackSource bs = b.getPackSource(); + + if (as == bs && DfsPackDescription.isGC(as)) { + // Push smaller GC files last; these likely have higher quality + // delta compression and the contained representation should be + // favored over other files. + return Long.signum(b.getFileSize(PACK) - a.getFileSize(PACK)); + } + + // DfsPackDescription.compareTo already did a reasonable sort. + // Rely on Arrays.sort being stable, leaving equal elements. + return 0; + }; + } + private final DfsRepositoryDescription repoDesc; private final String packName; private PackSource packSource; @@ -461,48 +543,6 @@ public class DfsPackDescription implements Comparable { return false; } - /** - * {@inheritDoc} - *

- * Sort packs according to the optimal lookup ordering. - *

- * This method tries to position packs in the order readers should examine - * them when looking for objects by SHA-1. The default tries to sort packs - * with more recent modification dates before older packs, and packs with - * fewer objects before packs with more objects. - */ - @Override - public int compareTo(DfsPackDescription b) { - // Cluster by PackSource, pushing UNREACHABLE_GARBAGE to the end. - PackSource as = getPackSource(); - PackSource bs = b.getPackSource(); - int cmp = PackSource.DEFAULT_COMPARATOR.compare(as, bs); - if (cmp != 0) { - return cmp; - } - - // Tie break GC type packs by smallest first. There should be at most - // one of each source, but when multiple exist concurrent GCs may have - // run. Preferring the smaller file selects higher quality delta - // compression, placing less demand on the DfsBlockCache. - if (as == bs && isGC(as)) { - cmp = Long.signum(getFileSize(PACK) - b.getFileSize(PACK)); - if (cmp != 0) { - return cmp; - } - } - - // Newer packs should sort first. - cmp = Long.signum(b.getLastModified() - getLastModified()); - if (cmp != 0) - return cmp; - - // Break ties on smaller index. Readers may get lucky and find - // the object they care about in the smaller index. This also pushes - // big historical packs to the end of the list, due to more objects. - return Long.signum(getObjectCount() - b.getObjectCount()); - } - static boolean isGC(PackSource s) { switch (s) { case GC: diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java index ec0a778e0..d04709f6c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java @@ -45,7 +45,6 @@ package org.eclipse.jgit.internal.storage.dfs; import static org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource.UNREACHABLE_GARBAGE; -import static org.eclipse.jgit.internal.storage.pack.PackExt.PACK; import static org.eclipse.jgit.lib.Constants.OBJECT_ID_LENGTH; import java.io.IOException; @@ -66,7 +65,6 @@ import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.StoredObjectRepresentationNotAvailableException; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackList; -import org.eclipse.jgit.internal.storage.dfs.DfsObjDatabase.PackSource; import org.eclipse.jgit.internal.storage.file.BitmapIndexImpl; import org.eclipse.jgit.internal.storage.file.PackBitmapIndex; import org.eclipse.jgit.internal.storage.file.PackIndex; @@ -611,26 +609,9 @@ public class DfsReader extends ObjectReader implements ObjectReuseAsIs { } } - private static final Comparator PACK_SORT_FOR_REUSE = new Comparator() { - @Override - public int compare(DfsPackFile af, DfsPackFile bf) { - DfsPackDescription ad = af.getPackDescription(); - DfsPackDescription bd = bf.getPackDescription(); - PackSource as = ad.getPackSource(); - PackSource bs = bd.getPackSource(); - - if (as == bs && DfsPackDescription.isGC(as)) { - // Push smaller GC files last; these likely have higher quality - // delta compression and the contained representation should be - // favored over other files. - return Long.signum(bd.getFileSize(PACK) - ad.getFileSize(PACK)); - } - - // DfsPackDescription.compareTo already did a reasonable sort. - // Rely on Arrays.sort being stable, leaving equal elements. - return 0; - } - }; + private static final Comparator PACK_SORT_FOR_REUSE = + Comparator.comparing( + DfsPackFile::getPackDescription, DfsPackDescription.reuseComparator()); private List sortPacksForSelectRepresentation() throws IOException {