Merge lp:~spiv/bzr/redundant-missing-keys-check into lp:bzr

[Andrew Bennetts]

Remove redundant parent inventories calculation during fetch.

This removes a parent inventories calculation from fetch.py. The reasons why I am confident this is safe are:

 * repo.insert_stream already performs this calculation to generate the "missing_keys" set it returns, and RepoFetcher._parent_inventories was adding to that same set.
 * it was only called for repositories with fallbacks, so there's no way that old formats with could be affected, so there's no risk to strange corner cases involving e.g. insert_stream on a knit repo.
 * when there are missing keys, a send insert_stream is performed, and the missing_keys result from that second call is asserted to be empty as a sanity check — yet that sanity check never calls self._parent_inventories. So this code was already trusting insert_stream to be sufficient.
 * by inspection, the parent inventories logic in insert_stream is identical (in fact, stricter: it performs the calculation if the repo supports stacking, even if it isn't stacked).
 * all the tests pass as is, except for one HPSS acceptance test, which improves by one round trip :)

As a result of deleting this redundant code the push-to-stacked HPSS ratchet tightens by 1 call: it removes a get_parent_map call that used to happen immediately after an insert_stream call.

I added a What's New entry that covers both this and the "Branch.lock_read caches tags" improvement made earlier in the 2.3 cycle.

[John A Meinel (jameinel)]

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

On 12/14/2010 3:38 AM, Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/redundant-missing-keys-check into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This removes a parent inventories calculation from fetch.py. The reasons why I am confident this is safe are:
>
> * repo.insert_stream already performs this calculation to generate the "missing_keys" set it returns, and RepoFetcher._parent_inventories was adding to that same set.
> * it was only called for repositories with fallbacks, so there's no way that old formats with could be affected, so there's no risk to strange corner cases involving e.g. insert_stream on a knit repo.
> * when there are missing keys, a send insert_stream is performed, and the missing_keys result from that second call is asserted to be empty as a sanity check — yet that sanity check never calls self._parent_inventories. So this code was already trusting insert_stream to be sufficient.
> * by inspection, the parent inventories logic in insert_stream is identical (in fact, stricter: it performs the calculation if the repo supports stacking, even if it isn't stacked).
> * all the tests pass as is, except for one HPSS acceptance test, which improves by one round trip :)
>
> As a result of deleting this redundant code the push-to-stacked HPSS ratchet tightens by 1 call: it removes a get_parent_map call that used to happen immediately after an insert_stream call.
>
> I added a What's New entry that covers both this and the "Branch.lock_read caches tags" improvement made earlier in the 2.3 cycle.

=== modified file 'bzrlib/fetch.py'
- --- bzrlib/fetch.py 2010-09-17 04:35:23 +0000
+++ bzrlib/fetch.py 2010-12-14 09:38:10 +0000
@@ -125,9 +125,6 @@
             pb.update("Inserting stream")
             resume_tokens, missing_keys = self.sink.insert_stream(
                 stream, from_format, [])
- - if self.to_repository._fallback_repositories:
- - missing_keys.update(
- - self._parent_inventories(search.get_keys()))
             if missing_keys:
                 pb.update("Missing keys")
                 stream = source.get_stream_for_missing_keys(missing_keys)

^- IIRC, this was added because we *weren't* sending missing keys
properly in the stream. So when we fixed it, we had both the client and
the server double check for extra missing keys.

I'm not positive that this is the fix, but in bzr-1.14.txt:
* Pushing a new stacked branch will also push the parent inventories for
  revisions at the stacking boundary. This makes sure that the stacked
  branch has enough data to calculate inventory deltas for all of its
  revisions (without requiring the fallback branch). This avoids
  "'AbsentContentFactory' object has no attribute 'get_bytes_as'" errors
  when fetching the stacked branch from a 1.13 (or later) smart server.
  This partially fixes #354036. (Andrew Bennetts, Robert Collins)

So we've only really had the bug in 1.6/1.9/1.14 era repos. It doesn't
look like it has been a problem for 2a repos.

I'm not as worried about maintaining strict workarounds for bzr-1.13 vs
not...

review: Approve

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'bzrlib/fetch.py'
--- bzrlib/fetch.py	2010-09-17 04:35:23 +0000
+++ bzrlib/fetch.py	2010-12-14 09:38:10 +0000
@@ -125,9 +125,6 @@
             pb.update("Inserting stream")
             resume_tokens, missing_keys = self.sink.insert_stream(
                 stream, from_format, [])
-            if self.to_repository._fallback_repositories:
-                missing_keys.update(
-                    self._parent_inventories(search.get_keys()))
             if missing_keys:
                 pb.update("Missing keys")
                 stream = source.get_stream_for_missing_keys(missing_keys)
@@ -163,18 +160,6 @@
             self.from_repository, self._last_revision,
             find_ghosts=self.find_ghosts)
 
-    def _parent_inventories(self, revision_ids):
-        # Find all the parent revisions referenced by the stream, but
-        # not present in the stream, and make sure we send their
-        # inventories.
-        parent_maps = self.to_repository.get_parent_map(revision_ids)
-        parents = set()
-        map(parents.update, parent_maps.itervalues())
-        parents.discard(NULL_REVISION)
-        parents.difference_update(revision_ids)
-        missing_keys = set(('inventories', rev_id) for rev_id in parents)
-        return missing_keys
-
 
 class Inter1and2Helper(object):
     """Helper for operations that convert data from model 1 and 2

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2010-12-09 04:26:53 +0000
+++ bzrlib/tests/blackbox/test_push.py	2010-12-14 09:38:10 +0000
@@ -231,7 +231,7 @@
         # being too low. If rpc_count increases, more network roundtrips have
         # become necessary for this use case. Please do not adjust this number
         # upwards without agreement from bzr's network support maintainers.
-        self.assertLength(14, self.hpss_calls)
+        self.assertLength(13, self.hpss_calls)
         remote = branch.Branch.open('public')
         self.assertEndsWith(remote.get_stacked_on_url(), '/parent')
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-12-07 16:27:49 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-12-14 09:38:10 +0000
@@ -26,6 +26,11 @@
 .. Improvements to existing commands, especially improved performance 
    or memory usage, or better results.
 
+* A redundant parent inventories calculation was removed from
+  ``fetch.py``, as ``Repository.insert_stream`` already reports any
+  missing inventories.  This removes at least one network roundtrip when
+  pushing to a stacked branch.  (Andrew Bennetts)
+
 * ``bzr resolve`` now accepts ``--take-this`` and ``--take-other`` actions
   for text conflicts. This *replace* the whole file with the content
   designated by the action. This will *ignore* all differences that would

=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
--- doc/en/whats-new/whats-new-in-2.3.txt	2010-12-03 07:51:28 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-12-14 09:38:10 +0000
@@ -69,6 +69,9 @@
 * ``bzr send`` uses less memory.
   (John Arbash Meinel, #614576)
 
+* Fetches involving stacked branches and branches with tags are now faster.
+  Some redundant network reads were removed.  (Andrew Bennetts)
+
 * Inventory entries now consume less memory (on 32-bit Ubuntu file entries
   have dropped from 68 bytes to 40, and directory entries from 120 bytes
   to 48).  This affects most operations, and depending on the size of the