Merge lp:~lifeless/bzr/bug-402652 into lp:~bzr/bzr/trunk-old

[Robert Collins]

[Robert Collins (lifeless)]

[MERGE] Fix bug 402652 by recompressing all texts that are streamed -
slightly slower at fetch, substantially faster and more compact at read.

--

[John A Meinel (jameinel)]

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

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/bug-402652 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> [MERGE] Fix bug 402652 by recompressing all texts that are streamed -
> slightly slower at fetch, substantially faster and more compact at read.
>
>

 review: abstain

I personally feel that this is going too far overboard. If you are doing
something like branching all of launchpad you will have:
60,000 revisions of perfectly packed data
and
1,000 revisions of poorly packed data (some of which is probably autopacked)

Telling the system that it must repack all 61,000 revisions on every
fetch is (IMO) significant overkill.

I think the current tradeoff (unordered fetching) is a better tradeoff
than that, until we get to the point where we can do a partial packing
on-the-fly.

By the way, is there an open bug for asking Launchpad to pack the
mirrored area? (You could force a pack of hosted bzr+ssh://
repositories, but that wouldn't do anything for the http side.)

John
=:->

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

iEYEARECAAYFAkqdhGsACgkQJdeBCYSNAAO+hQCfUqkujjGCsliSCBA49dER50v5
JAMAoJJiTu9v8SWcqMdIkAdyn64Y6Rln
=CadJ
-----END PGP SIGNATURE-----

review: Abstain

[John A Meinel (jameinel)]

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

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/bug-402652 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> [MERGE] Fix bug 402652 by recompressing all texts that are streamed -
> slightly slower at fetch, substantially faster and more compact at read.
>
>

A few actual review comments, while I'm looking at the code:

1) The test needs a self.addCleanup(target.unlock)
2) The change breaks some of the direct tests of insert_record_stream in
bzrlib/tests/test_groupcompress.py

3) I'm not sure what the 'if not insert_keys' stuff is. Maybe to make
those tests pass? insert_keys is only filled if random_id is sent, to
ensure that the ids sent are genuinely not repeated. (I've run into some
problems with the chk fetching code in the past when I set random_id =
True).

If it is there because of those tests, I think that is incorrect. But
I'd like to understand what you are trying to do.

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

iEYEARECAAYFAkqdj+kACgkQJdeBCYSNAANsKACfYbkJLci0fos+gOtvy72DTPEL
rAgAoIb7g7y28NWtztQbaZ9mMocmvjVO
=4vsm
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

I'm pretty sure this is superseded by the change to streaming fetch that I did. So I'm marking this as rejected. If I'm missing something, please re-open/resubmit.

review: Needs Resubmitting

[Robert Collins (lifeless)]

On Tue, 2009-10-06 at 18:01 +0000, John A Meinel wrote:
> Review: Resubmit
> I'm pretty sure this is superseded by the change to streaming fetch that I did. So I'm marking this as rejected. If I'm missing something, please re-open/resubmit.

It was the same thing, earlier version of :P

So reject is fine.

-Rob

=== modified file 'NEWS'
--- NEWS	2009-08-30 23:51:10 +0000
+++ NEWS	2009-09-01 07:35:08 +0000
@@ -69,6 +69,13 @@
   revisions that are in the fallback repository. (Regressed in 2.0rc1).
   (John Arbash Meinel, #419241)
 
+* Fetches from 2a to 2a are now again requested in 'groupcompress' order,
+  and all texts are recombined appropriately. This doesn't reuse existing
+  groups, which will be measurable in some specific circumstances - an
+  approximately 25% overhead. However, doing this ensures high performance
+  reads subsequent to the fetch operation, which is the most common
+  operation: write once read many. (Robert Collins, #402652)
+
 * Fix a segmentation fault when computing the ``merge_sort`` of a graph
   that has a ghost in the mainline ancestry.
   (John Arbash Meinel, #419241)

=== modified file 'bzrlib/groupcompress.py'
--- bzrlib/groupcompress.py	2009-08-30 21:34:42 +0000
+++ bzrlib/groupcompress.py	2009-09-01 07:35:08 +0000
@@ -1516,7 +1516,8 @@
         # test_insert_record_stream_existing_keys fail for groupcompress and
         # groupcompress-nograph, this needs to be revisited while addressing
         # 'bzr branch' performance issues.
-        for _ in self._insert_record_stream(stream, random_id=False):
+        for _ in self._insert_record_stream(stream, random_id=False,
+            reuse_blocks=False):
             pass
 
     def _insert_record_stream(self, stream, random_id=False, nostore_sha=None,
@@ -1580,7 +1581,7 @@
                                ' but then inserted %r two times', record.key)
                     continue
                 inserted_keys.add(record.key)
-            if reuse_blocks:
+            if not inserted_keys and reuse_blocks:
                 # If the reuse_blocks flag is set, check to see if we can just
                 # copy a groupcompress block as-is.
                 if record.storage_kind == 'groupcompress-block':

=== modified file 'bzrlib/repofmt/groupcompress_repo.py'
--- bzrlib/repofmt/groupcompress_repo.py	2009-08-24 19:34:13 +0000
+++ bzrlib/repofmt/groupcompress_repo.py	2009-09-01 07:35:08 +0000
@@ -932,7 +932,7 @@
         super(GroupCHKStreamSource, self).__init__(from_repository, to_format)
         self._revision_keys = None
         self._text_keys = None
-        # self._text_fetch_order = 'unordered'
+        self._text_fetch_order = 'groupcompress'
         self._chk_id_roots = None
         self._chk_p_id_roots = None
 
@@ -949,7 +949,7 @@
             p_id_roots_set = set()
             source_vf = self.from_repository.inventories
             stream = source_vf.get_record_stream(inventory_keys,
-                                                 'unordered', True)
+                                                 'groupcompress', True)
             for record in stream:
                 if record.storage_kind == 'absent':
                     if allow_absent:

=== modified file 'bzrlib/tests/test_repository.py'
--- bzrlib/tests/test_repository.py	2009-08-17 23:15:55 +0000
+++ bzrlib/tests/test_repository.py	2009-09-01 07:35:08 +0000
@@ -683,6 +683,27 @@
 
 class Test2a(TestCaseWithTransport):
 
+    def test_fetch_combines_groups(self):
+        builder = self.make_branch_builder('source', format='2a')
+        builder.start_series()
+        builder.build_snapshot('1', None, [
+            ('add', ('', 'root-id', 'directory', '')),
+            ('add', ('file', 'file-id', 'file', 'content\n'))])
+        builder.build_snapshot('2', ['1'], [
+            ('modify', ('file-id', 'content-2\n'))])
+        builder.finish_series()
+        source = builder.get_branch()
+        target = self.make_repository('target', format='2a')
+        target.fetch(source.repository)
+        target.lock_read()
+        details = target.texts._index.get_build_details(
+            [('file-id', '1',), ('file-id', '2',)])
+        file_1_details = details[('file-id', '1')]
+        file_2_details = details[('file-id', '2')]
+        # The index, and what to read off disk, should be the same for both
+        # versions of the file.
+        self.assertEqual(file_1_details[0][:3], file_2_details[0][:3])
+
     def test_format_pack_compresses_True(self):
         repo = self.make_repository('repo', format='2a')
         self.assertTrue(repo._format.pack_compresses)