Initial push slows down because of extra locking

[Martin Pool]

On 6/5/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> I was trying to debug pushing to an FTP server today with someone, and it
> revealed some interesting information about our initial push code and some of
> the reasons why it is slow.

That is interesting to see.  I've wanted for a while to add a debug
option that would print out all transport operations -- I guess you
just put this directly into the ftp transport for debugging?

To take that a bit further we could add some automatic checks that the
transport is being used reasonably - for example we should never need
to read the same file two times in a row, and could warn about that.

We could also have log debug code that counts how many times locks are
acquired and released, and both log this and also assert that for
particular tests it is less than some number.

> First, we end up locking and unlocking the target directories several times. I
> think that is because when we create a repository or branch we lock it while we
> are creating it, and then unlock it, only to lock it again to push data into it.
>
> Probably we should consider that "BranchFormat.create()" should return a
> write-locked object. It also seems to be caused because
> RepositoryFormat._upload_blank_content() is taking out a write lock, and copies
> up some basic files, and then *unlocks* right before we start doing real work.

Yes, I think we should do that for all the creation methods.  Possibly
we should give this a new name, since existing callers won't expect it
to be returned locked - on the other hand maybe we should just change
it, and the warning about not being unlocked will be a clue they
should be updated?


> ^- This is checking that we still held the lock and then we released it. I'm a
> little surprised that we didn't check the releasing.*/info file. Because
> otherwise we might break someone elses lock if they broke in at just the right
> time. But probably it isn't something we *need* to do.

The current code will tell us if our lock was broken just before we
release it; I think in that case we give an error and don't release
the lock.  You're right that we could check after we rename but before
we delete it, which would catch the case where someone else races in
and breaks and then replaces our lock between the get and the rename
-- that should normally be a small window but it is possible.  That
would let us tell "oops, we just released someone else's lock", and we
could try renaming it back but that might cause more trouble.  Then
two locks have been broken, which is a rather worse situation.  So I
think we must check before renaming, and we don't gain much from
checking after.

> But notice that we just
> created a ".bzr/*" directory, locked it, and then released the lock. I'm not
> sure why we are locking for .bzr/*

I don't think the control directory lock needs to be used for most
operations, since there is little mutable data in there.  It does seem
like we'd need it to protect against two processes simultaneously
trying to upgrade formats, and so probably need to hold it during
init.
> Anyway, overall we seem to be locking the repository 4 or 5 times rather than
> just 1 time. And the branch 2-3 times. There are a couple obvious things we
> could do to fix this, though I'm not sure how to make our apis do exactly the
> right thing. (Format.initialize() should return a locked object, and I still
> think we should create the branch and repository at the same time, so we could
> maintain a single lock across the whole fetch + create last-revision
> information for the branch). But still we could just do it by passing a locked
> Repository to the Branch so that it doesn't have to be locked yet again.
>
>
> This is actually one of the problems about making our API "nice" and auto-lock
> for us. We don't realize when we are locking and unlocking more often than we
> really should.

I agree, and I think this is also a bit of a problem with the nice
decorators - obviously saving typing is good, but it seems a bit too
easy to just add them on because they might be needed.

-- 
Martin