RepoView: Make flushing inserter a no-op

Flushing is already done automatically at the right time by the
BatchUpdate implementations, and the RevWalk provided to callers can
already read unflushed objects. A BatchUpdateOp implementation calling
flush on the inserter is doing something wrong, and flushing too much
can seriously impact DFS performance when it happens in a loop.

Make flushing (and closing) a no-op rather than slapping implementors'
wrists by throwing an exception. Even if the implementor is well-
intentioned, we can't guarantee that all downstream library functions
are going to know not to flush their inserters. For example, JGit's
Merger used to be aggressive about flushing until I fixed it[1]. This
was the most glaring example, but I would be utterly unsurprised to find
more.

Also don't use InMemoryInserter for this purpose, as it actually flushes
too little. The purpose of that class is to avoid writing a pack to DFS
when the internal buffer in DfsInserter exceeds some limit, but in
BatchUpdate, it's fine to write out an intermediate pack in this case,
we don't have to buffer the whole thing in memory.

[1] https://git.eclipse.org/r/12504

Change-Id: I2b139e50af213c4b683c25d30b190bc547304f72
This commit is contained in:
Dave Borowitz
2017-04-07 12:43:58 -04:00
parent df009c66f9
commit 3915d7baa1
4 changed files with 32 additions and 7 deletions

View File

@@ -332,11 +332,6 @@ public abstract class BatchUpdate implements AutoCloseable {
return repoView.getRevWalk();
}
protected ObjectInserter getObjectInserter() throws IOException {
initRepository();
return repoView.getInserter();
}
public Map<String, ReceiveCommand> getRefUpdates() {
return repoView != null ? repoView.getCommands().getCommands() : ImmutableMap.of();
}

View File

@@ -184,7 +184,7 @@ class NoteDbBatchUpdate extends BatchUpdate {
private class RepoContextImpl extends ContextImpl implements RepoContext {
@Override
public ObjectInserter getInserter() throws IOException {
return getRepoView().getInserter();
return getRepoView().getInserterWrapper();
}
@Override

View File

@@ -48,12 +48,14 @@ public class RepoView {
private final Repository repo;
private final RevWalk rw;
private final ObjectInserter inserter;
private final ObjectInserter inserterWrapper;
private final ChainedReceiveCommands commands;
private final boolean closeRepo;
RepoView(GitRepositoryManager repoManager, Project.NameKey project) throws IOException {
repo = repoManager.openRepository(project);
inserter = repo.newObjectInserter();
inserterWrapper = new NonFlushingInserter(inserter);
rw = new RevWalk(inserter.newReader());
commands = new ChainedReceiveCommands(repo);
closeRepo = true;
@@ -68,6 +70,7 @@ public class RepoView {
this.repo = checkNotNull(repo);
this.rw = checkNotNull(rw);
this.inserter = checkNotNull(inserter);
inserterWrapper = new NonFlushingInserter(inserter);
commands = new ChainedReceiveCommands(repo);
closeRepo = false;
}
@@ -198,7 +201,34 @@ public class RepoView {
return inserter;
}
ObjectInserter getInserterWrapper() {
return inserterWrapper;
}
ChainedReceiveCommands getCommands() {
return commands;
}
private static class NonFlushingInserter extends ObjectInserter.Filter {
private final ObjectInserter delegate;
private NonFlushingInserter(ObjectInserter delegate) {
this.delegate = delegate;
}
@Override
protected ObjectInserter delegate() {
return delegate;
}
@Override
public void flush() {
// Do nothing.
}
@Override
public void close() {
// Do nothing; the delegate is closed separately.
}
}
}

View File

@@ -155,7 +155,7 @@ class ReviewDbBatchUpdate extends BatchUpdate {
private class RepoContextImpl extends ContextImpl implements RepoContext {
@Override
public ObjectInserter getInserter() throws IOException {
return getRepoView().getInserter();
return getRepoView().getInserterWrapper();
}
@Override