Vast number of round-trips pushing stacked branch

[Andrew Bennetts]

Pushing a stacked branch takes a ridiculous number of round-trips. Here's an extreme demonstration, pushing bzr.dev stacked onto a branch with just r1 of bzr.dev:

andrew@steerpike:~/warthogs/bzr/devel$ bzr init bzr://localhost:4156/repo2/r1 --1.6
Repository branch (format: unnamed)
Location:
  shared repository: bzr://localhost:4156/repo2/
  repository branch: bzr://localhost:4156/repo2/r1/

andrew@steerpike:~/warthogs/bzr/devel$ bzr -Dhpss push bzr://localhost:4156/repo2/r1 -r1
Pushed up to revision 1.
HPSS calls: 41 <bzrlib.smart.medium.SmartTCPClientMedium object at 0xa7d024c>

andrew@steerpike:~/warthogs/bzr/devel$ time bzr --no-plugins -Dhpss push bzr://localhost:4156/repo2/stacked --stacked --stacked-on bzr://localhost:4156/repo2/r1
Source format does not support stacking, using format: '1.6'
  Packs 5 (adds stacking support, requires bzr 1.6)

Created new stacked branch referring to bzr://localhost:4156/repo2/r1.
HPSS calls: 10 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x97884cc>
HPSS calls: 15 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x8aaaf6c>
HPSS calls: 93585 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x879980c>
HPSS calls: 11 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x97d360c>

real 3m9.360s
user 2m5.604s
sys 0m7.272s

Most of those HPSS calls are tiny writes to the new pack file:

28.899 hpss call w/body: 'append', '/repo2/stacked/.bzr/repository/upload/h7tn7to10hkupvrhl0yu.pack', '' ('B237\n\n\x1f\x8b\x08\x00\x00\x00\x00\x00\x02\xff\x9d\x90\xcdj'...)
28.899 243 bytes
28.900 result: ('appended', '3233112')
28.901 hpss call w/body: 'append', '/repo2/stacked/.bzr/repository/upload/h7tn7to10hkupvrhl0yu.pack', '' ('B748\n\n\x1f\x8b\x08\x00\x00\x00\x00\x00\x02\xff\x9dT\xd1n'...)
28.901 754 bytes
28.901 result: ('appended', '3233355')
28.902 hpss call w/body: 'append', '/repo2/stacked/.bzr/repository/upload/h7tn7to10hkupvrhl0yu.pack', '' ('B182\n\n\x1f\x8b\x08\x00\x00\x00\x00\x00\x02\xff\x9dOK\n'...)
28.902 188 bytes
28.902 result: ('appended', '3234109')
28.903 hpss call w/body: 'append', '/repo2/stacked/.bzr/repository/upload/h7tn7to10hkupvrhl0yu.pack', '' ('B457\n\n\x1f\x8b\x08\x00\x00\x00\x00\x00\x02\xff\xa5\x93Q\x8f'...)
28.903 463 bytes
28.903 result: ('appended', '3234297')

[Robert Collins (lifeless)]

On Thu, 2008-11-06 at 02:09 +0000, Andrew Bennetts wrote:
>
> Most of those HPSS calls are tiny writes to the new pack file:

I'm sure I've noted before that the hpss file stream is not as efficient
as sftp's :P

Tossing a cache into the object should help :)

Alternatively/possibly-as-well having a thread do actual writes would
introduce more concurrency

-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.

[Robert Collins (lifeless)]

Oh, but of course - remember that this won't be used by a streaming push
per-se :P. So the other route is to see why the packer object has no
cache size set.

-Rob

[Andrew Bennetts (spiv)]

Well, it has no cache size set because nothing sets it ;)

InterPackRepo falls back on the generic RepoFetcher when there are fallback repositories. The NewPack is created by the start_write_group() that does, and nothing ever sets the cache size.

A simple hack would be to parameterise RepoFetcher a bit more, so that InterPackRepo can give it a function to run after the start_write_group(). I've attached a patch that does this, and it does give much better behaviour:

"
HPSS calls: 15 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x91790ac>
HPSS calls: 10 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x9f48bac>
HPSS calls: 11 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x9feae6c>
HPSS calls: 203 <bzrlib.smart.medium.SmartTCPClientMedium object at 0x8f2b72c>

real 1m58.151s
user 1m18.913s
sys 0m2.164s
"

[Robert Collins (lifeless)]

probably cleaner to just make the default 1MB

-Rob

[Andrew Bennetts (spiv)]

Doing that does seem simpler, but it causes test failures — some code seems to expect to be able to read bytes from packs that are being created.

There's a comment in the NewPack constructor that says:

        # How much data to cache when writing packs. Note that this is not
        # synchronised with reads, because it's not in the transport layer, so
        # is not safe unless the client knows it won't be reading from the pack
        # under creation.
        self._cache_limit = 0

So I'll send my more complex fix to the mailing list for now, if only to make it more visible.

[Martin Pool (mbp)]

patch posted

http://bundlebuggy.aaronbentley.com/project/bzr/request/%3C20081112233139.GA28617%40steerpike.home.puzzling.org%3E

[Andrew Bennetts (spiv)]

My hackish fix for this is in 1.10rc1 (and bzr.dev). It'd be good to fix it more elegantly later, but at least users of stacked branches don't have to suffer in the mean time.

[Martin Pool (mbp)]

Andrew's patch for this caused bug 303856 so I'm going to reverse it.

[Andrew Bennetts (spiv)]

If you upgrade your client and server to 1.13, then this bug is already fixed.

I've also sent a patch to the list that will fix it for clients talking to pre-1.13 servers too.