From 5cf53fdacf28d5cabe7ad1ed154fe7f4971225a9 Mon Sep 17 00:00:00 2001 From: Robin Rosenberg Date: Mon, 18 Feb 2013 20:25:00 +0100 Subject: [PATCH] Speed up clone/fetch with large number of refs Instead of re-reading all refs after each update, execute the deletes first, then read all refs once and perform the check for conflicting ref names in memory. Change-Id: I17d0b3ccc27f868c8497607d8e57bf7082e65ba3 --- .../storage/file/RefDirectoryTest.java | 115 +++++++++++++++++- .../org/eclipse/jgit/lib/BatchRefUpdate.java | 111 ++++++++++++++--- .../src/org/eclipse/jgit/lib/RefUpdate.java | 16 ++- 3 files changed, 223 insertions(+), 19 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java index 7d39d0900..9bf3b94c4 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java @@ -60,6 +60,8 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -67,17 +69,20 @@ import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jgit.events.ListenerHandle; import org.eclipse.jgit.events.RefsChangedEvent; import org.eclipse.jgit.events.RefsChangedListener; -import org.eclipse.jgit.internal.storage.file.FileRepository; -import org.eclipse.jgit.internal.storage.file.RefDirectory; import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.AnyObjectId; +import org.eclipse.jgit.lib.BatchRefUpdate; +import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Ref.Storage; import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevTag; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; +import org.eclipse.jgit.transport.ReceiveCommand.Type; import org.junit.Before; import org.junit.Test; @@ -1175,6 +1180,112 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { assertEquals(1, changeCount.get()); } + @Test + public void testBatchRefUpdateSimpleNoForce() throws IOException { + writeLooseRef("refs/heads/master", A); + writeLooseRef("refs/heads/masters", B); + List commands = Arrays.asList( + newCommand(A, B, "refs/heads/master", + ReceiveCommand.Type.UPDATE), + newCommand(B, A, "refs/heads/masters", + ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); + BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); + batchUpdate.addCommand(commands); + batchUpdate + .execute(new RevWalk(diskRepo), NullProgressMonitor.INSTANCE); + Map refs = refdir.getRefs(RefDatabase.ALL); + assertEquals(ReceiveCommand.Result.OK, commands.get(0).getResult()); + assertEquals(ReceiveCommand.Result.REJECTED_NONFASTFORWARD, commands + .get(1).getResult()); + assertEquals("[HEAD, refs/heads/master, refs/heads/masters]", refs + .keySet().toString()); + assertEquals(B.getId(), refs.get("refs/heads/master").getObjectId()); + assertEquals(B.getId(), refs.get("refs/heads/masters").getObjectId()); + } + + @Test + public void testBatchRefUpdateSimpleForce() throws IOException { + writeLooseRef("refs/heads/master", A); + writeLooseRef("refs/heads/masters", B); + List commands = Arrays.asList( + newCommand(A, B, "refs/heads/master", + ReceiveCommand.Type.UPDATE), + newCommand(B, A, "refs/heads/masters", + ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); + BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); + batchUpdate.setAllowNonFastForwards(true); + batchUpdate.addCommand(commands); + batchUpdate + .execute(new RevWalk(diskRepo), NullProgressMonitor.INSTANCE); + Map refs = refdir.getRefs(RefDatabase.ALL); + assertEquals(ReceiveCommand.Result.OK, commands.get(0).getResult()); + assertEquals(ReceiveCommand.Result.OK, commands.get(1).getResult()); + assertEquals("[HEAD, refs/heads/master, refs/heads/masters]", refs + .keySet().toString()); + assertEquals(B.getId(), refs.get("refs/heads/master").getObjectId()); + assertEquals(A.getId(), refs.get("refs/heads/masters").getObjectId()); + } + + @Test + public void testBatchRefUpdateConflict() throws IOException { + writeLooseRef("refs/heads/master", A); + writeLooseRef("refs/heads/masters", B); + List commands = Arrays.asList( + newCommand(A, B, "refs/heads/master", + ReceiveCommand.Type.UPDATE), + newCommand(null, A, "refs/heads/master/x", + ReceiveCommand.Type.CREATE), + newCommand(null, A, "refs/heads", ReceiveCommand.Type.CREATE)); + BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); + batchUpdate.setAllowNonFastForwards(true); + batchUpdate.addCommand(commands); + batchUpdate + .execute(new RevWalk(diskRepo), NullProgressMonitor.INSTANCE); + Map refs = refdir.getRefs(RefDatabase.ALL); + assertEquals(ReceiveCommand.Result.OK, commands.get(0).getResult()); + assertEquals(ReceiveCommand.Result.LOCK_FAILURE, commands.get(1) + .getResult()); + assertEquals(ReceiveCommand.Result.LOCK_FAILURE, commands.get(2) + .getResult()); + assertEquals("[HEAD, refs/heads/master, refs/heads/masters]", refs + .keySet().toString()); + assertEquals(B.getId(), refs.get("refs/heads/master").getObjectId()); + assertEquals(B.getId(), refs.get("refs/heads/masters").getObjectId()); + } + + @Test + public void testBatchRefUpdateConflictThanksToDelete() throws IOException { + writeLooseRef("refs/heads/master", A); + writeLooseRef("refs/heads/masters", B); + List commands = Arrays.asList( + newCommand(A, B, "refs/heads/master", + ReceiveCommand.Type.UPDATE), + newCommand(null, A, "refs/heads/masters/x", + ReceiveCommand.Type.CREATE), + newCommand(B, null, "refs/heads/masters", + ReceiveCommand.Type.DELETE)); + BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); + batchUpdate.setAllowNonFastForwards(true); + batchUpdate.addCommand(commands); + batchUpdate + .execute(new RevWalk(diskRepo), NullProgressMonitor.INSTANCE); + Map refs = refdir.getRefs(RefDatabase.ALL); + assertEquals(ReceiveCommand.Result.OK, commands.get(0).getResult()); + assertEquals(ReceiveCommand.Result.OK, commands.get(1).getResult()); + assertEquals(ReceiveCommand.Result.OK, commands.get(2).getResult()); + assertEquals("[HEAD, refs/heads/master, refs/heads/masters/x]", refs + .keySet().toString()); + assertEquals(A.getId(), refs.get("refs/heads/masters/x").getObjectId()); + } + + private static ReceiveCommand newCommand(RevCommit a, RevCommit b, + String string, Type update) { + ReceiveCommand ret = new ReceiveCommand(a != null ? a.getId() : null, + b != null ? b.getId() : null, string, update); + ret.setResult(ReceiveCommand.Result.NOT_ATTEMPTED); + return ret; + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { writeLooseRef(name, id.name() + "\n"); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java index 98c264719..b86d6fad8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java @@ -53,9 +53,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; import org.eclipse.jgit.internal.JGitText; +import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.ReceiveCommand; @@ -242,41 +244,120 @@ public class BatchRefUpdate { * @param walk * a RevWalk to parse tags in case the storage system wants to * store them pre-peeled, a common performance optimization. - * @param update + * @param monitor * progress monitor to receive update status on. * @throws IOException * the database is unable to accept the update. Individual * command status must be tested to determine if there is a * partial failure, or a total failure. */ - public void execute(RevWalk walk, ProgressMonitor update) + public void execute(RevWalk walk, ProgressMonitor monitor) throws IOException { - update.beginTask(JGitText.get().updatingReferences, commands.size()); + monitor.beginTask(JGitText.get().updatingReferences, commands.size()); + List commands2 = new ArrayList( + commands.size()); + List namesToCheck = new ArrayList(commands.size()); + // First delete refs. This may free the name space for some of the + // updates. for (ReceiveCommand cmd : commands) { try { - update.update(1); - if (cmd.getResult() == NOT_ATTEMPTED) { cmd.updateType(walk); - RefUpdate ru = newUpdate(cmd); switch (cmd.getType()) { - case DELETE: - cmd.setResult(ru.delete(walk)); - continue; - case CREATE: + namesToCheck.add(cmd.getRefName()); + commands2.add(cmd); + break; case UPDATE: case UPDATE_NONFASTFORWARD: - cmd.setResult(ru.update(walk)); - continue; + commands2.add(cmd); + break; + case DELETE: + RefUpdate rud = newUpdate(cmd); + monitor.update(1); + cmd.setResult(rud.delete(walk)); } } } catch (IOException err) { - cmd.setResult(REJECTED_OTHER_REASON, MessageFormat.format( - JGitText.get().lockError, err.getMessage())); + cmd.setResult( + REJECTED_OTHER_REASON, + MessageFormat.format(JGitText.get().lockError, + err.getMessage())); + } + } + if (!commands2.isEmpty()) { + // What part of the name space is already taken + Collection takenNames = new HashSet(refdb.getRefs( + RefDatabase.ALL).keySet()); + Collection takenPrefixes = getTakenPrefixes(takenNames); + + // Now to the update that may require more room in the name space + for (ReceiveCommand cmd : commands2) { + try { + monitor.update(1); + + if (cmd.getResult() == NOT_ATTEMPTED) { + cmd.updateType(walk); + RefUpdate ru = newUpdate(cmd); + SWITCH: switch (cmd.getType()) { + case DELETE: + // Performed in the first phase + break; + case UPDATE: + case UPDATE_NONFASTFORWARD: + monitor.update(1); + RefUpdate ruu = newUpdate(cmd); + cmd.setResult(ruu.update(walk)); + break; + case CREATE: + for (String prefix : getPrefixes(cmd.getRefName())) { + if (takenNames.contains(prefix)) { + cmd.setResult(Result.LOCK_FAILURE); + break SWITCH; + } + } + if (takenPrefixes.contains(cmd.getRefName())) { + cmd.setResult(Result.LOCK_FAILURE); + break SWITCH; + } + ru.setCheckConflicting(false); + addRefToPrefixes(takenPrefixes, cmd.getRefName()); + takenNames.add(cmd.getRefName()); + cmd.setResult(ru.update(walk)); + } + } + } catch (IOException err) { + cmd.setResult(REJECTED_OTHER_REASON, MessageFormat.format( + JGitText.get().lockError, err.getMessage())); + } } } - update.endTask(); + monitor.endTask(); + } + + private static Collection getTakenPrefixes( + final Collection names) { + Collection ref = new HashSet(); + for (String name : names) + ref.addAll(getPrefixes(name)); + return ref; + } + + private static void addRefToPrefixes(Collection prefixes, + String name) { + for (String prefix : getPrefixes(name)) { + prefixes.add(prefix); + } + } + + static Collection getPrefixes(String s) { + Collection ret = new HashSet(); + int p1 = s.indexOf('/'); + while (p1 > 0) { + ret.add(s.substring(0, p1)); + p1 = s.indexOf('/', p1 + 1); + } + return ret; } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java index fcf38e695..56ec7af89 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java @@ -178,6 +178,8 @@ public abstract class RefUpdate { */ private boolean detachingSymbolicRef; + private boolean checkConflicting = true; + /** * Construct a new update operation for the reference. *

@@ -564,7 +566,7 @@ public abstract class RefUpdate { public Result link(String target) throws IOException { if (!target.startsWith(Constants.R_REFS)) throw new IllegalArgumentException(MessageFormat.format(JGitText.get().illegalArgumentNotA, Constants.R_REFS)); - if (getRefDatabase().isNameConflicting(getName())) + if (checkConflicting && getRefDatabase().isNameConflicting(getName())) return Result.LOCK_FAILURE; try { if (!tryLock(false)) @@ -599,7 +601,7 @@ public abstract class RefUpdate { RevObject oldObj; // don't make expensive conflict check if this is an existing Ref - if (oldValue == null && getRefDatabase().isNameConflicting(getName())) + if (oldValue == null && checkConflicting && getRefDatabase().isNameConflicting(getName())) return Result.LOCK_FAILURE; try { if (!tryLock(true)) @@ -631,6 +633,16 @@ public abstract class RefUpdate { } } + /** + * Enable/disable the check for conflicting ref names. By default conflicts + * are checked explicitly. + * + * @param check + */ + public void setCheckConflicting(boolean check) { + checkConflicting = check; + } + private static RevObject safeParse(final RevWalk rw, final AnyObjectId id) throws IOException { try {