[MERGE] Prevent knit corruption by checking parents

[John Arbash Meinel]

Aaron Bentley wrote:
> Aaron Bentley wrote:
>>> This patch causes a KnitTextsDiffer exception to be raised when knits
>>> disagree about the content of a parent.
> 
> Updated so that KnitTextsDiffer is a printable exception...
> 
> Aaron

I'm a little concerned about the performance implications of this
change, because we do have to extract the parent contents in order to
find the sha1 sum.

However, it does look like you are restricting the check to a small set,
which seems like the best that we can do. (We don't actually have to
extract the entire gzip hunk, just the first line, but we probably have
to read the whole chunk, which is the real performance penalty over a
remote connection).

'bzr push' is already pretty slow, and now, on top of pushing data, we
have to read back data, and verify the sha1 sums.

I ran the benchmarks, and there is definitely an effect:

bzr.dev:
bench_sftp.SFTPBenchmark.test_push_1              OK    96ms/11943ms
bench_sftp.SFTPBenchmark.test_push_10             OK   116ms/13671ms
bench_sftp.SFTPBenchmark.test_push_100            OK   169ms/24482ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_1    OK  6584ms/18544ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_10   OK  6696ms/20157ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_100  OK  7540ms/31815ms

Now, I do believe that these are all very linear pushes (which will
probably be the most common type of push. It will happen, but be rare
that you do a push with merges where the target has some of the merged
revisions already)

check-parents:
bench_sftp.SFTPBenchmark.test_push_1               OK   116ms/12260ms
bench_sftp.SFTPBenchmark.test_push_10              OK   131ms/13087ms
bench_sftp.SFTPBenchmark.test_push_100             OK   202ms/23509ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_1     OK  7120ms/19820ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_10    OK  7244ms/20513ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_100   OK  8064ms/31539ms

So, locally, it is costing us 20-40ms to do this. Which I am okay with.
However, over the remote connection, it is costing 540ms. Which equates
to something like 18 round trips (at the 30ms latency of SocketDelay). I
assume that it has something to do with the number of files affected, etc.

So I like the integrity checking, but I'm concerned about how much
overhead it generates. Because of that, I'm not comfortable giving it
the green light without having more people look over it.

It also has an effect on the 'bzr pull' time. I'm not sure why it seems
to have a larger effect on pull than on push. Also, these tests show me
that my work on 'push' seems to have helped, we need to also start
focusing on 'bzr pull' time, considering that right now push is faster
than pull. (which I really would not have expected).

bzr.dev:
bench_sftp.SFTPBenchmark.test_pull_1               OK    69ms/12252ms
bench_sftp.SFTPBenchmark.test_pull_10              OK   288ms/14171ms
bench_sftp.SFTPBenchmark.test_pull_100             OK   941ms/38229ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_1     OK  2311ms/15679ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_10    OK 16375ms/32076ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_100   OK 20868ms/61207ms

check-parents:
bench_sftp.SFTPBenchmark.test_pull_1               OK    88ms/12392ms
bench_sftp.SFTPBenchmark.test_pull_10              OK   377ms/14119ms
bench_sftp.SFTPBenchmark.test_pull_100             OK  1029ms/38449ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_1     OK  2909ms/15591ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_10    OK 21424ms/35852ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_100   OK 25802ms/64020ms

That is a cost of 5s on 'pull_100', or a 25% slowdown.

John
=:->

PS> As a point of reference, bzr.dev is doing about the same as
bzr-0.10. There are some small differences, but most is within the noise
margin. (I believe locking has been improved slightly)

bench_sftp.SFTPBenchmark.test_pull_1               OK    67ms/10441ms
bench_sftp.SFTPBenchmark.test_pull_10              OK   314ms/12279ms
bench_sftp.SFTPBenchmark.test_pull_100             OK  1038ms/35593ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_1     OK  2296ms/14095ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_10    OK 16411ms/29531ms
bench_sftp.SFTPSlowSocketBenchmark.test_pull_100   OK 20730ms/56994ms

bench_sftp.SFTPBenchmark.test_push_1               OK   100ms/10718ms
bench_sftp.SFTPBenchmark.test_push_10              OK   109ms/11502ms
bench_sftp.SFTPBenchmark.test_push_100             OK   173ms/20085ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_1     OK  6740ms/18420ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_10    OK  6844ms/19588ms
bench_sftp.SFTPSlowSocketBenchmark.test_push_100   OK  7688ms/28755ms


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060922/e6867df7/attachment.pgp