From d9f84b0b7c49d23f2e064abed2cb4fa30354870d Mon Sep 17 00:00:00 2001 From: Minh Thai Date: Tue, 21 Apr 2020 21:17:58 -0700 Subject: [PATCH] UploadPack: Clear advertised ref map after negotiation After negotiation phase of a fetch, the advertised ref map is no longer used and can be safely cleared. For >1GiB repos object selection and packfile writing may take 10s of minutes. For the chromium.googlesource.com/chromium/src repo, this advertised ref map is >400MiB. Returning this memory to the Java heap is a major scalability win. Change-Id: I00d453c5ef47630c21f199e333e1cfcf47b7e92a Signed-off-by: Minh Thai --- .../jgit/transport/UploadPackTest.java | 78 +++++++++++++++++++ .../eclipse/jgit/transport/UploadPack.java | 13 +++- 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java index ea86563da..d58e57698 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java @@ -44,6 +44,7 @@ import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefDatabase; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.Sets; import org.eclipse.jgit.lib.TextProgressMonitor; @@ -2238,4 +2239,81 @@ public class UploadPackTest { } } + @Test + public void testSafeToClearRefsInFetchV0() throws Exception { + server = + new RefCallsCountingRepository( + new DfsRepositoryDescription("server")); + remote = new TestRepository<>(server); + RevCommit one = remote.commit().message("1").create(); + remote.update("one", one); + testProtocol = new TestProtocol<>((Object req, Repository db) -> { + UploadPack up = new UploadPack(db); + return up; + }, null); + uri = testProtocol.register(ctx, server); + try (Transport tn = testProtocol.open(uri, client, "server")) { + tn.fetch(NullProgressMonitor.INSTANCE, + Collections.singletonList(new RefSpec(one.name()))); + } + assertTrue(client.getObjectDatabase().has(one.toObjectId())); + assertEquals(1, ((RefCallsCountingRepository)server).numRefCalls()); + } + + @Test + public void testSafeToClearRefsInFetchV2() throws Exception { + server = + new RefCallsCountingRepository( + new DfsRepositoryDescription("server")); + remote = new TestRepository<>(server); + RevCommit one = remote.commit().message("1").create(); + RevCommit two = remote.commit().message("2").create(); + remote.update("one", one); + remote.update("two", two); + server.getConfig().setBoolean("uploadpack", null, "allowrefinwant", true); + ByteArrayInputStream recvStream = uploadPackV2( + "command=fetch\n", + PacketLineIn.delimiter(), + "want-ref refs/heads/one\n", + "want-ref refs/heads/two\n", + "done\n", + PacketLineIn.end()); + PacketLineIn pckIn = new PacketLineIn(recvStream); + assertThat(pckIn.readString(), is("wanted-refs")); + assertThat( + Arrays.asList(pckIn.readString(), pckIn.readString()), + hasItems( + one.toObjectId().getName() + " refs/heads/one", + two.toObjectId().getName() + " refs/heads/two")); + assertTrue(PacketLineIn.isDelimiter(pckIn.readString())); + assertThat(pckIn.readString(), is("packfile")); + parsePack(recvStream); + assertTrue(client.getObjectDatabase().has(one.toObjectId())); + assertEquals(1, ((RefCallsCountingRepository)server).numRefCalls()); + } + + private class RefCallsCountingRepository extends InMemoryRepository { + private final InMemoryRepository.MemRefDatabase refdb; + private int numRefCalls; + + public RefCallsCountingRepository(DfsRepositoryDescription repoDesc) { + super(repoDesc); + refdb = new InMemoryRepository.MemRefDatabase() { + @Override + public List getRefs() throws IOException { + numRefCalls++; + return super.getRefs(); + } + }; + } + + public int numRefCalls() { + return numRefCalls; + } + + @Override + public RefDatabase getRefDatabase() { + return refdb; + } + } } 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 35196c6e3..bb826d881 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -298,7 +298,7 @@ public class UploadPack { private boolean sentReady; - /** Objects we sent in our advertisement list, clients can ask for these. */ + /** Objects we sent in our advertisement list. */ private Set advertised; /** Marked on objects the client has asked us to give them. */ @@ -381,8 +381,10 @@ public class UploadPack { /** * Get refs which were advertised to the client. * - * @return all refs which were advertised to the client, or null if - * {@link #setAdvertisedRefs(Map)} has not been called yet. + * @return all refs which were advertised to the client. Only valid during + * the negotiation phase. Will return {@code null} if + * {@link #setAdvertisedRefs(Map)} has not been called yet or if + * {@code #sendPack()} has been called. */ public final Map getAdvertisedRefs() { return refs; @@ -2205,6 +2207,11 @@ public class UploadPack { } msgOut.flush(); + // Advertised objects and refs are not used from here on and can be + // cleared. + advertised = null; + refs = null; + PackConfig cfg = packConfig; if (cfg == null) cfg = new PackConfig(db);