Merge lp:~jameinel/bzr/2.1-static-tuple-chk-map into lp:bzr

This is an update to my earlier chk_map work (which Andrew had approved.)

The main changes are

1) I have benchmarked it as being specifically helpful. Namely it saves 30MB during 'bzr branch launchpad'. (548MB w/ bzr.dev => 518MB)

2) I changed it temporarily to require StaticTuples everywhere, and then updated the code in 'inventory.py' and 'groupcompress_repo.py' that were touching things directly.

3) I then updated the interface so that LeafNode and InternalNode and things underneath them require exactly StaticTuples, but CHKMap itself will cast to StaticTuple on demand. This seemed a fair way to migrate. Production code should never be poking at the internals of the nodes, and instead work via the CHKMap api.

This should help avoid spurious failures in the short term, and still encourage good habits in the long term.

I just tried this branch (well, + r4762 of bzr.dev), on my client and
server. Pushing to the server gave a traceback:

Thu 2009-10-22 03:35:30 +0000
0.357 bzr arguments: [u'serve', u'--inet', u'--directory=/',
u'--allow-writes']
0.372 looking for plugins in /home/mnordhoff/.bazaar/plugins
0.372 looking for plugins in /usr/local/co/bzr/bzr/bzr.dev/bzrlib/plugins
0.568 looking for plugins in
/usr/lib/python2.5/site-packages/bzrlib/plugins
0.571 encoding stdout as osutils.get_user_encoding() 'UTF-8'
2.439 bzr-svn: using Subversion 1.4.6 ()
10.883 Traceback (most recent call last):
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/smart/request.py", line
317, in _call_converting_errors
    return callable(*args, **kwargs)
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/smart/repository.py", line
727, in _inserter_thread
    stream, src_format, self.tokens)
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repository.py", line 4242,
in insert_stream
    return self._locked_insert_stream(stream, src_format, is_resume)
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repository.py", line 4343,
in _locked_insert_stream
    hint = self.target_repo.commit_write_group()
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repository.py", line 1557,
in commit_write_group
    result = self._commit_write_group()
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repofmt/pack_repo.py", line
2269, in _commit_write_group
    hint = self._pack_collection._commit_write_group()
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repofmt/pack_repo.py", line
2097, in _commit_write_group
    problems = self._check_new_inventories()
  File
"/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repofmt/groupcompress_repo.py",
line 653, in _check_new_inventories
    for record in _filter_text_keys(chk_diff, text_keys, bytes_to_info):
  File
"/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repofmt/groupcompress_repo.py",
line 1189, in _filter_text_keys
    for record, items in interesting_nodes_iterable:
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1642, in
process
    for record in self._read_all_roots():
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1566, in
_read_all_roots
    self._read_nodes_from_store(new_keys):
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1500, in
_read_nodes_from_store
    search_key_func=self._search_key_func)
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1438, in
_deserialise
    search_key_func=search_key_func)
  File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1023, in
deserialise
    ' StaticTuple not %s' % (type(key),))
AssertionError: deserialise should be called with a StaticTuple not
<type 'tuple'>

A local "bzr merge" did too:

Thu 2009-10-22 03:40:07 +0000
0.035 bzr arguments: [u'merge', u'/srv/bzr/bzr/statictuple-pickling/']
0.047 looking for plugins in /home/mnordhoff/.bazaar/plugins
0.047 looking for plugins in /usr/local/co/bzr/bzr/bzr.dev/bzrlib/plugins
0.162 looking for plugins in
/usr/lib/python2.5/site-packages/bzrlib/plugins
0.218 opening working tree '/usr/local/co/bzr/bzr/bzr.dev'
0.288 Using fetch logic to copy between
CHKInventoryRepository('file:///srv/bzr/bzr/.bzr/repository/')(<RepositoryF...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> I just tried this branch (well, + r4762 of bzr.dev), on my client and
> server. Pushing to the server gave a traceback:

Thanks for the heads up.

...

> line 653, in _check_new_inventories
> for record in _filter_text_keys(chk_diff, text_keys, bytes_to_info):
> File
> "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/repofmt/groupcompress_repo.py",
> line 1189, in _filter_text_keys
> for record, items in interesting_nodes_iterable:
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1642, in
> process
> for record in self._read_all_roots():
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1566, in
> _read_all_roots
> self._read_nodes_from_store(new_keys):
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1500, in
> _read_nodes_from_store
> search_key_func=self._search_key_func)
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1438, in
> _deserialise
> search_key_func=search_key_func)
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1023, in
> deserialise
> ' StaticTuple not %s' % (type(key),))
> AssertionError: deserialise should be called with a StaticTuple not
> <type 'tuple'>

I'll try to track down where the plain 'tuple' object came into play,
and also why I didn't catch it with the test suite. Admittedly I only
ran a subset, but I thought I ran "selftest -s bt.per_repo' which should
have covered this.

...

> for result in self.target.inventory.iter_changes(self.source.inventory):
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/inventory.py", line 2085,
> in iter_changes
> self.id_to_entry.iter_changes(basis.id_to_entry):
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 294, in
> iter_changes
> self._ensure_root()
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 140, in
> _ensure_root
> self._root_node = self._get_node(self._root_node)
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 155, in
> _get_node
> search_key_func=self._search_key_func)
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1421, in
> _deserialise
> search_key_func=search_key_func)
> File "/usr/local/co/bzr/bzr/bzr.dev/bzrlib/chk_map.py", line 1008, in
> deserialise
> search_key_func=search_key_func)
> File "_chk_map_pyx.pyx", line 368, in
> _chk_map_pyx._deserialise_internal_node
> TypeError: key ('sha1:e12a0f03e145bbabf18cc7b933cce82edfc005dd',) is not
> a StaticTuple
>
> Turning off plugins did not help the latter one; I didn't try it with
> the first one.

^- This is pretty surprising, I'll certainly give it a look. Namely, it
looks like the root_key in 'basis.id_to_entry' is not a StaticTuple,
which is surprising given that the code that sets the root key has:
if type(node) is tuple:
    node = StaticTuple.from_sequence(node)

Thanks for looking closely.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrgd48ACgkQJdeBCYSNAAOyVgCeIVTtO7/TDyp9nEv9CBUX4oq+
SM0An09eTMf4fXhfMSWtHcj48HGxKTQl
=oe2X
-----END PGP SIG...

pulling this back out since it seems to have failures, and I certainly can't trust PQM to catch them right now :)

Unfortunately, I'm not able to reproduce either of Matt's failures. At this point, *my* best guess is that he had not recompiled the extensions yet, so it wasn't getting all of the code paths corrected. I certainly could be wrong, but I haven't found how yet.

John A Meinel wrote:
> Unfortunately, I'm not able to reproduce either of Matt's failures. At this point, *my* best guess is that he had not recompiled the extensions yet, so it wasn't getting all of the code paths corrected. I certainly could be wrong, but I haven't found how yet.

That's totally possible. At the time, keeping track of which changes I
was making to bzr.dev was really starting to drive me batty.

Making sure to recompile everything, I just tried to reproduce the
"merge" error and couldn't.

Pushing to a test branch (since I don't have any other revisions to
push), I can't reproduce that error, either.

...Ah, I found some real data to push. Still can't reproduce it.

Sorry for the false alarm.
--

Ok, so I'm resubmitting this one again, only this time, it's even better.

Including the changes to CHKMap to only use StaticTuple internally, I also found that _filter_text_keys was creating a whole lot of file_key tuples, and not interning the various attributes.

I also found that we create *millions* of integer objects, and most of them are redundant because of the identical 'group' start and end information. (IIRC, we create 1.4M integers at peak of parsing the chk stream, and only 300k of them are unique.)

the _filter_text_keys fix saved around 40MB peak, interning the integers saves another 7MB.

Overall, with this patch, I'm now down to 457MB peak when branching all of launchpad. Which is very close to my 50% goal. I also know a way to save another ~10MB or so, but it requires using SimpleSet, which I'm not sure I want to do yet.

Anyway, versus bzr.dev, this patch drops me from 548MB => 457MB peak memory.

Also, I've focused a bit on 'streaming' data out of a repository (versus the insert on the other side). In that scenario, the numbers are:
  583MB bzr 2.0.1
  422MB bzr.dev
  338MB this patch

So not quite 50% savings, but I expect it to still be fairly noticable on Launchpad's code hosting.

Looks ok, it's mostly mechanical despite the large line count. A couple of questions:

772 - self.parent_id_basename_to_file_id.key())
773 + (self.parent_id_basename_to_file_id.key()[0],))

That change (and the other similar ones looks a bit odd.

Oh, is that how you're casting a StaticTuple to a tuple for string formatting? I would have thought "x.as_tuple()" would be about as fast as "(x[0],)", and it's certainly more readable. I suppose it is noticeably slower?

1115 -class TestSearchKeyFuncs(tests.TestCase):

Why delete this TestCase?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> Looks ok, it's mostly mechanical despite the large line count. A couple of questions:
>
> 772 - self.parent_id_basename_to_file_id.key())
> 773 + (self.parent_id_basename_to_file_id.key()[0],))
>
> That change (and the other similar ones looks a bit odd.

The old code was formatting with:

"foo %s" % key

Which happened to work because 'key' was a tuple, but I don't think it
was actually *intentional*. I think it was written thinking that the
object was a string / simply formatted. I have the habit of always using
%(,) formatting, just in case the object I'm using happens to end up as
a tuple.
And the rest is just returning the simple sha1: string, rather than a
'tuple' (or StaticTuple).

I can certainly use '.as_tuple()' if you feel that is better. I've
always felt that the string formatting syntax *should* be
'str %s' % (arg1, arg2)

To make sure you don't pass an arg you don't quite understand. Sort of
the same idea that you should never do:

fprintf(file, str)

but alway

fprintf(file, "%s", str)

the former will often work, until it breaks horribly.

>
> Oh, is that how you're casting a StaticTuple to a tuple for string formatting? I would have thought "x.as_tuple()" would be about as fast as "(x[0],)", and it's certainly more readable. I suppose it is noticeably slower?
>
> 1115 -class TestSearchKeyFuncs(tests.TestCase):
>
> Why delete this TestCase?

Because all of the tests are covered in the "test__chk_map" which is
also permuted against the C and python versions of _chk_map

John
=;->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrlAgIACgkQJdeBCYSNAAMnsQCbBdRB5bnNB4CxRTjgL9Gwy5wY
sowAnjco9oE2eSg6i03FOXgmHQUdtRPa
=/cVN
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Approve
> Looks ok, it's mostly mechanical despite the large line count. A couple of questions:
>
> 772 - self.parent_id_basename_to_file_id.key())
> 773 + (self.parent_id_basename_to_file_id.key()[0],))
>
> That change (and the other similar ones looks a bit odd.
>
> Oh, is that how you're casting a StaticTuple to a tuple for string formatting? I would have thought "x.as_tuple()" would be about as fast as "(x[0],)", and it's certainly more readable. I suppose it is noticeably slower?
>
> 1115 -class TestSearchKeyFuncs(tests.TestCase):
>
> Why delete this TestCase?

I'll mention, I'm not particularly comfortable landing this until we
sort how to get PQM actually failing appropriately again. I'm pretty
sure it is good, and I'll run the tests here, but it is a more-invasive
change. I suppose Babune will give me the real fallout?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrlAjIACgkQJdeBCYSNAANgyQCZARSGYStiFrFUxiNnSsyoW2Si
MzoAn10O3EG6v2BN00kMZrRApjZ7SZ3e
=pRWD
-----END PGP SIGNATURE-----

On Mon, 2009-10-26 at 02:00 +0000, John A Meinel wrote:
>
>
> I'll mention, I'm not particularly comfortable landing this until we
> sort how to get PQM actually failing appropriately again. I'm pretty
> sure it is good, and I'll run the tests here, but it is a
> more-invasive
> change. I suppose Babune will give me the real fallout?

Yes, it will.

-Rob