Merge lp:~jelmer/bzr/hpss-get-inventories into lp:bzr

[Jelmer Vernooij]

Add HPSS call for retrieving inventories.

Add a HPSS call for ``Repository.iter_inventories``.

This massively reduces the number of roundtrips for various commands, and causes a fair number to no longer use VFS calls at all when used against a modern remote server.

Repository.get_deltas_for_revisions() now has its own implementation for remote repositories, which means the client won't use the VFS to get at the inventories to generate the deltas from.

Unlike my previous attempt at this, this now uses record streams with inventory deltas. It is still a separate verb though.

[Jelmer Vernooij (jelmer)]

Timings when checking out bzr.dev:

~/src/bzr/hpss-get-inventories/bzr co --lightweight bzr://people.samba.org/bzr.dev 2.25s user 0.46s system 15% cpu 17.277 total
~/src/bzr/hpss-get-inventories/bzr export /tmp/bzr.dev2 bzr://people.samba.org/bzr.dev 1.54s user 0.40s system 12% cpu 15.435 total

versus against an older bzr server:

~/src/bzr/hpss-get-inventories/bzr co --lightweight bzr://people.samba.org/bzr.dev 4.91s user 1.03s system 10% cpu 58.807 total
~/src/bzr/hpss-get-inventories/bzr export /tmp/bzr.dev5 bzr://people.samba.org/bzr.dev 4.35s user 0.98s system 9% cpu 58.357 total

[Jelmer Vernooij (jelmer)]

For comparison (though I should it's a different server):

apt-get source bzr 2.79s user 0.54s system 27% cpu 12.338 total

so we're getting closer.

[Andrew Bennetts (spiv)]

Jelmer Vernooij wrote:
> Add a HPSS call for ``Repository.iter_inventories``.

I am a bit concerned that by adding verbs like this, with their own ad hoc
record stream-like wire format, that we're growing the maintenance burden
unnecessarily, and not reusing improvements everywhere we could.

I'm glad this is using inventory-deltas. But if it's worth zlib compressing
them here, shouldn't we do that for inventory-deltas from get_record_stream too?

Also, could you implement this via the get_record_stream RPC with a new
parameter that means “just inventories (rather than rev+inv+texts+chks+sigs for
given keys)”?

On one hand it might feel a bit ugly to make one RPC do so many things,
get_record_stream, do so many things, but on the other I think it provides a
useful pressure to keep the interface minimal and consistent (e.g. whether
records are zlib-compressed).

Adding this iter_inventories RPC might be the right thing (although putting
“iter” in the name of an RPC feels weird to me, that's surely a property of the
Python API rather than a property of the RPC), but I'd like hear what you think
about the tradeoffs of doing that vs. extending get_record_stream.

-Andrew.

[Martin Pool (mbp)]

On 30 November 2011 09:37, Andrew Bennetts
<email address hidden> wrote:
> Jelmer Vernooij wrote:
>> Add a HPSS call for ``Repository.iter_inventories``.
>
> I am a bit concerned that by adding verbs like this, with their own ad hoc
> record stream-like wire format, that we're growing the maintenance burden
> unnecessarily, and not reusing improvements everywhere we could.

+1

--
Martin

[Vincent Ladeuil (vila)]

>> I am a bit concerned that by adding verbs like this, with their own ad hoc
>> record stream-like wire format, that we're growing the maintenance burden
>> unnecessarily, and not reusing improvements everywhere we could.

> +1

I kind of had the same feeling when jelmer started adding a bunch of verbs that were basically replacing a vfs roundtrip by a smart request roundtrip.

BUT

If we stop maintaining these verbs on the server side, the clients will fallback to vfs.

So we introduce different/better verbs, we can remove the old ones in both the server and the client and all but the newest clients can still fallback to vfs.

The net effect is at least to get to a point where the most recent client/server do not use vfs at all.

This sounds like a good incremental step to me.

'reusing improvements everywhere we could' is still valuable but doesn't to block progress.

review: Approve

[Jelmer Vernooij (jelmer)]

Hi Andrew,

> Jelmer Vernooij wrote:
> > Add a HPSS call for ``Repository.iter_inventories``.
> I am a bit concerned that by adding verbs like this, with their own ad hoc
> record stream-like wire format, that we're growing the maintenance burden
> unnecessarily, and not reusing improvements everywhere we could.
>
> I'm glad this is using inventory-deltas. But if it's worth zlib compressing
> them here, shouldn't we do that for inventory-deltas from get_record_stream
> too?
It's a pretty big inventory delta in this case - for something like gcc it matters for the delta between null: and the initial revision that was requested. I haven't investigated whether it would help for record streams too, but I imagine it would have less of an effect.

> Also, could you implement this via the get_record_stream RPC with a new
> parameter that means “just inventories (rather than rev+inv+texts+chks+sigs
> for
> given keys)”?
>
> On one hand it might feel a bit ugly to make one RPC do so many things,
> get_record_stream, do so many things, but on the other I think it provides a
> useful pressure to keep the interface minimal and consistent (e.g. whether
> records are zlib-compressed).
>
> Adding this iter_inventories RPC might be the right thing (although putting
> “iter” in the name of an RPC feels weird to me, that's surely a property of
> the Python API rather than a property of the RPC), but I'd like hear what you
> think about the tradeoffs of doing that vs. extending get_record_stream.

With regard to the name: Most of the other calls seem to be named after the equivalent method in the client / server, that's why I went with Repository.iter_inventories. I'm not particularly tied to that name, something like "Repository.get_inventories" or "Repository.stream_inventories" seems reasonable to me too.

I'm not sure whether this should be part of get_record_stream. I have a hard time understanding the get_record_stream code as it is, so I'd rather not make it even more complex by adding more parameters - for example, get_record_streams seem to be dependent on the repository on-disk format to an extent - the inventory stream is not, as it's using inventory deltas. In other words, adding another verb was simpler, while keeping it all understandable for a mere mortal like me. :-)

Either way, I agree we should be reusing more code between the work I've done recently and the existing record stream
 calls. But it seems to me that would best be done by refactoring so they e.g. use the same code for sending a stream of blobs of indeterminate length, rather than by all being a part of the same verb.

What do you think?

Cheers,

Jelmer

[Andrew Bennetts (spiv)]

Jelmer Vernooij wrote:
…
> > I'm glad this is using inventory-deltas. But if it's worth zlib compressing
> > them here, shouldn't we do that for inventory-deltas from get_record_stream
> > too?
> It's a pretty big inventory delta in this case - for something like gcc it
> matters for the delta between null: and the initial revision that was
> requested. I haven't investigated whether it would help for record streams
> too, but I imagine it would have less of an effect.

Well, I know that there are already cases that send deltas from null: —
something to do with stacking perhaps? So reusing this improvement globally
would be nice.

> > Also, could you implement this via the get_record_stream RPC with a new
> > parameter that means “just inventories (rather than rev+inv+texts+chks+sigs
> > for
> > given keys)”?
> >
> > On one hand it might feel a bit ugly to make one RPC do so many things,
> > get_record_stream, do so many things, but on the other I think it provides a
> > useful pressure to keep the interface minimal and consistent (e.g. whether
> > records are zlib-compressed).
> >
> > Adding this iter_inventories RPC might be the right thing (although putting
> > “iter” in the name of an RPC feels weird to me, that's surely a property of
> > the Python API rather than a property of the RPC), but I'd like hear what you
> > think about the tradeoffs of doing that vs. extending get_record_stream.
>
> With regard to the name: Most of the other calls seem to be named after the
> equivalent method in the client / server, that's why I went with
> Repository.iter_inventories. I'm not particularly tied to that name, something
> like "Repository.get_inventories" or "Repository.stream_inventories" seems
> reasonable to me too.

I think of using the name of the Python API as a good guide, not a strict rule.
Certainly there's no insert_stream_locked method on Repository…

“Repository.get_inventories” sounds fine to me.

> I'm not sure whether this should be part of get_record_stream. I have a hard
> time understanding the get_record_stream code as it is, so I'd rather not make
> it even more complex by adding more parameters - for example,
> get_record_streams seem to be dependent on the repository on-disk format to an
> extent - the inventory stream is not, as it's using inventory deltas. In other
> words, adding another verb was simpler, while keeping it all understandable
> for a mere mortal like me. :-)
>
> Either way, I agree we should be reusing more code between the work I've done
> recently and the existing record stream calls. But it seems to me that would
> best be done by refactoring so they e.g. use the same code for sending a
> stream of blobs of indeterminate length, rather than by all being a part of
> the same verb.

Well, the path to reuse we've developed *is* record streams. I'm quite willing
to believe that get_record_stream's parameters aren't a convenient way to
express all needs. But I'd really like it the thing that was returned by a new
verb was a true record stream — something you could pass directly to
insert_record_stream. The idea is that record streams should be the One True
Way we have to say “here is a stream of da...

[Jelmer Vernooij (jelmer)]

Am 05/12/11 11:31, schrieb Andrew Bennetts:
> Jelmer Vernooij wrote:
> …
>>> I'm glad this is using inventory-deltas. But if it's worth zlib compressing
>>> them here, shouldn't we do that for inventory-deltas from get_record_stream
>>> too?
>> It's a pretty big inventory delta in this case - for something like gcc it
>> matters for the delta between null: and the initial revision that was
>> requested. I haven't investigated whether it would help for record streams
>> too, but I imagine it would have less of an effect.
> Well, I know that there are already cases that send deltas from null: —
> something to do with stacking perhaps? So reusing this improvement globally
> would be nice.
I guess the best way to do this would be to add a zlib pack record kind
for network streams? I.e. a new entry in
NetworkRecordStream._kind_factory, and something equivalent on the
server side?
>>> Also, could you implement this via the get_record_stream RPC with a new
>>> parameter that means “just inventories (rather than rev+inv+texts+chks+sigs
>>> for
>>> given keys)”?
>>>
>>> On one hand it might feel a bit ugly to make one RPC do so many things,
>>> get_record_stream, do so many things, but on the other I think it provides a
>>> useful pressure to keep the interface minimal and consistent (e.g. whether
>>> records are zlib-compressed).
>>>
>>> Adding this iter_inventories RPC might be the right thing (although putting
>>> “iter” in the name of an RPC feels weird to me, that's surely a property of
>>> the Python API rather than a property of the RPC), but I'd like hear what you
>>> think about the tradeoffs of doing that vs. extending get_record_stream.
>> With regard to the name: Most of the other calls seem to be named after the
>> equivalent method in the client / server, that's why I went with
>> Repository.iter_inventories. I'm not particularly tied to that name, something
>> like "Repository.get_inventories" or "Repository.stream_inventories" seems
>> reasonable to me too.
> I think of using the name of the Python API as a good guide, not a strict rule.
> Certainly there's no insert_stream_locked method on Repository…
>
> “Repository.get_inventories” sounds fine to me.
I've renamed it.
>
>> I'm not sure whether this should be part of get_record_stream. I have a hard
>> time understanding the get_record_stream code as it is, so I'd rather not make
>> it even more complex by adding more parameters - for example,
>> get_record_streams seem to be dependent on the repository on-disk format to an
>> extent - the inventory stream is not, as it's using inventory deltas. In other
>> words, adding another verb was simpler, while keeping it all understandable
>> for a mere mortal like me. :-)
>>
>> Either way, I agree we should be reusing more code between the work I've done
>> recently and the existing record stream calls. But it seems to me that would
>> best be done by refactoring so they e.g. use the same code for sending a
>> stream of blobs of indeterminate length, rather than by all being a part of
>> the same verb.
> Well, the path to reuse we've developed *is* record streams. I'm quite willing
> to believe that get_record_stream's parameters aren'...

[Vincent Ladeuil (vila)]

Based on the tests and the decrease of roundtrips: approved !

16 + def _vfs_checkout_metadir(self):
17 + self._ensure_real()
18 + return self._real_bzrdir.checkout_metadir()
19 +
20 + def checkout_metadir(self):
21 + medium = self._client._medium
22 + if medium._is_remote_before((2, 5)):
23 + return self._vfs_checkout_metadir()

_vfs_checkout_metadir -> checkout_metadir -> _vfs_checkout_metadir ?

Is that a loop or do I miss something obvious ?

Up to you to wait for spiv's review.

review: Approve

[Jelmer Vernooij (jelmer)]

Am 13/12/11 11:14, schrieb Vincent Ladeuil:
> Review: Approve
>
> Based on the tests and the decrease of roundtrips: approved !
>
> 16 + def _vfs_checkout_metadir(self):
> 17 + self._ensure_real()
> 18 + return self._real_bzrdir.checkout_metadir()
> 19 +
> 20 + def checkout_metadir(self):
> 21 + medium = self._client._medium
> 22 + if medium._is_remote_before((2, 5)):
> 23 + return self._vfs_checkout_metadir()
>
> _vfs_checkout_metadir -> checkout_metadir -> _vfs_checkout_metadir ?
It's not checkout_metadir, but _real_bzrdir.checkout_metadir. :-)
>
> Is that a loop or do I miss something obvious ?
>
> Up to you to wait for spiv's review.
Cheers,

Jelmer

[Jelmer Vernooij (jelmer)]

> Up to you to wait for spiv's review.
Thanks.

I haven't heard from spiv in the last week, and I'd really like to get this landed so it gets some decent testing for 2.5.0. My recent changes should have addressed his concerns regarding the use of record streams, and I'm happy to tweak whatever is necessary later based on post-commit reviews.

review: Approve

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2011-12-14 12:33:04 +0000
+++ bzrlib/remote.py	2011-12-14 21:28:31 +0000
@@ -27,6 +27,7 @@
     errors,
     gpg,
     graph,
+    inventory_delta,
     lock,
     lockdir,
     osutils,
@@ -1830,9 +1831,125 @@
     def get_inventory(self, revision_id):
         return list(self.iter_inventories([revision_id]))[0]
 
+    def _iter_inventories_rpc(self, revision_ids, ordering):
+        if ordering is None:
+            ordering = 'unordered'
+        path = self.bzrdir._path_for_remote_call(self._client)
+        body = "\n".join(revision_ids)
+        response_tuple, response_handler = (
+            self._call_with_body_bytes_expecting_body(
+                "VersionedFileRepository.get_inventories",
+                (path, ordering), body))
+        if response_tuple[0] != "ok":
+            raise errors.UnexpectedSmartServerResponse(response_tuple)
+        deserializer = inventory_delta.InventoryDeltaDeserializer()
+        byte_stream = response_handler.read_streamed_body()
+        decoded = smart_repo._byte_stream_to_stream(byte_stream)
+        if decoded is None:
+            # no results whatsoever
+            return
+        src_format, stream = decoded
+        if src_format.network_name() != self._format.network_name():
+            raise AssertionError(
+                "Mismatched RemoteRepository and stream src %r, %r" % (
+                src_format.network_name(), self._format.network_name()))
+        # ignore the src format, it's not really relevant
+        prev_inv = Inventory(root_id=None,
+            revision_id=_mod_revision.NULL_REVISION)
+        # there should be just one substream, with inventory deltas
+        substream_kind, substream = stream.next()
+        if substream_kind != "inventory-deltas":
+            raise AssertionError(
+                 "Unexpected stream %r received" % substream_kind)
+        for record in substream:
+            (parent_id, new_id, versioned_root, tree_references, invdelta) = (
+                deserializer.parse_text_bytes(record.get_bytes_as("fulltext")))
+            if parent_id != prev_inv.revision_id:
+                raise AssertionError("invalid base %r != %r" % (parent_id,
+                    prev_inv.revision_id))
+            inv = prev_inv.create_by_apply_delta(invdelta, new_id)
+            yield inv, inv.revision_id
+            prev_inv = inv
+
+    def _iter_inventories_vfs(self, revision_ids, ordering=None):
+        self._ensure_real()
+        return self._real_repository._iter_inventories(revision_ids, ordering)
+
     def iter_inventories(self, revision_ids, ordering=None):
-        self._ensure_real()
-        return self._real_repository.iter_inventories(revision_ids, ordering)
+        """Get many inventories by revision_ids.
+
+        This will buffer some or all of the texts used in constructing the
+        inventories in memory, but will only parse a single inventory at a
+        time.
+
+        :param revision_ids: The expected revision ids of the inventories.
+        :param ordering: optional ordering, e.g. 'topological'.  If not
+            specified, the order of revision_ids will be preserved (by
+            buffering if necessary).
+        :return: An iterator of inventories.
+        """
+        if ((None in revision_ids)
+            or (_mod_revision.NULL_REVISION in revision_ids)):
+            raise ValueError('cannot get null revision inventory')
+        for inv, revid in self._iter_inventories(revision_ids, ordering):
+            if inv is None:
+                raise errors.NoSuchRevision(self, revid)
+            yield inv
+
+    def _iter_inventories(self, revision_ids, ordering=None):
+        if len(revision_ids) == 0:
+            return
+        missing = set(revision_ids)
+        if ordering is None:
+            order_as_requested = True
+            invs = {}
+            order = list(revision_ids)
+            order.reverse()
+            next_revid = order.pop()
+        else:
+            order_as_requested = False
+            if ordering != 'unordered' and self._fallback_repositories:
+                raise ValueError('unsupported ordering %r' % ordering)
+        iter_inv_fns = [self._iter_inventories_rpc] + [
+            fallback._iter_inventories for fallback in
+            self._fallback_repositories]
+        try:
+            for iter_inv in iter_inv_fns:
+                request = [revid for revid in revision_ids if revid in missing]
+                for inv, revid in iter_inv(request, ordering):
+                    if inv is None:
+                        continue
+                    missing.remove(inv.revision_id)
+                    if ordering != 'unordered':
+                        invs[revid] = inv
+                    else:
+                        yield inv, revid
+                if order_as_requested:
+                    # Yield as many results as we can while preserving order.
+                    while next_revid in invs:
+                        inv = invs.pop(next_revid)
+                        yield inv, inv.revision_id
+                        try:
+                            next_revid = order.pop()
+                        except IndexError:
+                            # We still want to fully consume the stream, just
+                            # in case it is not actually finished at this point
+                            next_revid = None
+                            break
+        except errors.UnknownSmartMethod:
+            for inv, revid in self._iter_inventories_vfs(revision_ids, ordering):
+                yield inv, revid
+            return
+        # Report missing
+        if order_as_requested:
+            if next_revid is not None:
+                yield None, next_revid
+            while order:
+                revid = order.pop()
+                yield invs.get(revid), revid
+        else:
+            while missing:
+                yield None, missing.pop()
 
     @needs_read_lock
     def get_revision(self, revision_id):
@@ -2191,6 +2308,8 @@
 
     @needs_read_lock
     def _get_inventory_xml(self, revision_id):
+        # This call is used by older working tree formats,
+        # which stored a serialized basis inventory.
         self._ensure_real()
         return self._real_repository._get_inventory_xml(revision_id)
 
@@ -2235,11 +2354,58 @@
             revids.update(set(fallback.all_revision_ids()))
         return list(revids)
 
+    def _filtered_revision_trees(self, revision_ids, file_ids):
+        """Return Tree for a revision on this branch with only some files.
+
+        :param revision_ids: a sequence of revision-ids;
+          a revision-id may not be None or 'null:'
+        :param file_ids: if not None, the result is filtered
+          so that only those file-ids, their parents and their
+          children are included.
+        """
+        inventories = self.iter_inventories(revision_ids)
+        for inv in inventories:
+            # Should we introduce a FilteredRevisionTree class rather
+            # than pre-filter the inventory here?
+            filtered_inv = inv.filter(file_ids)
+            yield InventoryRevisionTree(self, filtered_inv, filtered_inv.revision_id)
+
     @needs_read_lock
     def get_deltas_for_revisions(self, revisions, specific_fileids=None):
-        self._ensure_real()
-        return self._real_repository.get_deltas_for_revisions(revisions,
-            specific_fileids=specific_fileids)
+        medium = self._client._medium
+        if medium._is_remote_before((1, 2)):
+            self._ensure_real()
+            for delta in self._real_repository.get_deltas_for_revisions(
+                    revisions, specific_fileids):
+                yield delta
+            return
+        # Get the revision-ids of interest
+        required_trees = set()
+        for revision in revisions:
+            required_trees.add(revision.revision_id)
+            required_trees.update(revision.parent_ids[:1])
+
+        # Get the matching filtered trees. Note that it's more
+        # efficient to pass filtered trees to changes_from() rather
+        # than doing the filtering afterwards. changes_from() could
+        # arguably do the filtering itself but it's path-based, not
+        # file-id based, so filtering before or afterwards is
+        # currently easier.
+        if specific_fileids is None:
+            trees = dict((t.get_revision_id(), t) for
+                t in self.revision_trees(required_trees))
+        else:
+            trees = dict((t.get_revision_id(), t) for
+                t in self._filtered_revision_trees(required_trees,
+                specific_fileids))
+
+        # Calculate the deltas
+        for revision in revisions:
+            if not revision.parent_ids:
+                old_tree = self.revision_tree(_mod_revision.NULL_REVISION)
+            else:
+                old_tree = trees[revision.parent_ids[0]]
+            yield trees[revision.revision_id].changes_from(old_tree)
 
     @needs_read_lock
     def get_revision_delta(self, revision_id, specific_fileids=None):

=== modified file 'bzrlib/smart/repository.py'
--- bzrlib/smart/repository.py	2011-12-11 13:30:10 +0000
+++ bzrlib/smart/repository.py	2011-12-14 21:28:31 +0000
@@ -14,7 +14,7 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
-"""Server-side repository related request implmentations."""
+"""Server-side repository related request implementations."""
 
 import bz2
 import os
@@ -28,6 +28,8 @@
     bencode,
     errors,
     estimate_compressed_size,
+    inventory as _mod_inventory,
+    inventory_delta,
     osutils,
     pack,
     trace,
@@ -43,6 +45,7 @@
 from bzrlib.repository import _strip_NULL_ghosts, network_format_registry
 from bzrlib import revision as _mod_revision
 from bzrlib.versionedfile import (
+    ChunkedContentFactory,
     NetworkRecordStream,
     record_to_fulltext_bytes,
     )
@@ -1243,3 +1246,57 @@
                 yield zlib.compress(record.get_bytes_as('fulltext'))
         finally:
             self._repository.unlock()
+
+
+class SmartServerRepositoryGetInventories(SmartServerRepositoryRequest):
+    """Get the inventory deltas for a set of revision ids.
+
+    This accepts a list of revision ids, and then sends a chain
+    of deltas for the inventories of those revisions. The first
+    revision will be empty.
+
+    The server writes back zlibbed serialized inventory deltas,
+    in the ordering specified. The base for each delta is the
+    inventory generated by the previous delta.
+
+    New in 2.5.
+    """
+
+    def _inventory_delta_stream(self, repository, ordering, revids):
+        prev_inv = _mod_inventory.Inventory(root_id=None,
+            revision_id=_mod_revision.NULL_REVISION)
+        serializer = inventory_delta.InventoryDeltaSerializer(
+            repository.supports_rich_root(),
+            repository._format.supports_tree_reference)
+        repository.lock_read()
+        try:
+            for inv, revid in repository._iter_inventories(revids, ordering):
+                if inv is None:
+                    continue
+                inv_delta = inv._make_delta(prev_inv)
+                lines = serializer.delta_to_lines(
+                    prev_inv.revision_id, inv.revision_id, inv_delta)
+                yield ChunkedContentFactory(inv.revision_id, None, None, lines)
+                prev_inv = inv
+        finally:
+            repository.unlock()
+
+    def body_stream(self, repository, ordering, revids):
+        substream = self._inventory_delta_stream(repository,
+            ordering, revids)
+        return _stream_to_byte_stream([('inventory-deltas', substream)],
+            repository._format)
+
+    def do_body(self, body_bytes):
+        return SuccessfulSmartServerResponse(('ok', ),
+            body_stream=self.body_stream(self._repository, self._ordering,
+                body_bytes.splitlines()))
+
+    def do_repository_request(self, repository, ordering):
+        if ordering == 'unordered':
+            # inventory deltas for a topologically sorted stream
+            # are likely to be smaller
+            ordering = 'topological'
+        self._ordering = ordering
+        # Signal that we want a body
+        return None

=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py	2011-12-14 12:20:36 +0000
+++ bzrlib/smart/request.py	2011-12-14 21:28:31 +0000
@@ -751,15 +751,18 @@
     'Repository.check_write_group', 'bzrlib.smart.repository',
     'SmartServerRepositoryCheckWriteGroup', info='read')
 request_handlers.register_lazy(
-    'VersionedFileRepository.get_serializer_format', 'bzrlib.smart.repository',
-    'SmartServerRepositoryGetSerializerFormat', info='read')
-request_handlers.register_lazy(
     'Repository.reconcile', 'bzrlib.smart.repository',
     'SmartServerRepositoryReconcile', info='idem')
 request_handlers.register_lazy(
     'Repository.tarball', 'bzrlib.smart.repository',
     'SmartServerRepositoryTarball', info='read')
 request_handlers.register_lazy(
+    'VersionedFileRepository.get_serializer_format', 'bzrlib.smart.repository',
+    'SmartServerRepositoryGetSerializerFormat', info='read')
+request_handlers.register_lazy(
+    'VersionedFileRepository.get_inventories', 'bzrlib.smart.repository',
+    'SmartServerRepositoryGetInventories', info='read')
+request_handlers.register_lazy(
     'rmdir', 'bzrlib.smart.vfs', 'RmdirRequest', info='semivfs')
 request_handlers.register_lazy(
     'stat', 'bzrlib.smart.vfs', 'StatRequest', info='read')

=== modified file 'bzrlib/tests/blackbox/test_annotate.py'
--- bzrlib/tests/blackbox/test_annotate.py	2011-12-14 12:15:44 +0000
+++ bzrlib/tests/blackbox/test_annotate.py	2011-12-14 21:28:31 +0000
@@ -326,6 +326,6 @@
         # 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(19, self.hpss_calls)
+        self.assertLength(15, self.hpss_calls)
         self.expectFailure("annotate accesses inventories, which require VFS access",
             self.assertThat, self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2011-12-14 18:17:43 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2011-12-14 21:28:31 +0000
@@ -483,7 +483,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(40, self.hpss_calls)
+        self.assertLength(33, self.hpss_calls)
         self.expectFailure("branching to the same branch requires VFS access",
             self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
 
@@ -521,8 +521,8 @@
         # 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.assertThat(self.hpss_calls, ContainsNoVfsCalls)
         self.assertLength(15, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
 
     def test_branch_from_branch_with_tags(self):
         self.setup_smart_server_with_call_log()
@@ -539,8 +539,8 @@
         # 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(10, self.hpss_calls)
         self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
-        self.assertLength(10, self.hpss_calls)
 
     def test_branch_to_stacked_from_trivial_branch_streaming_acceptance(self):
         self.setup_smart_server_with_call_log()

=== modified file 'bzrlib/tests/blackbox/test_cat.py'
--- bzrlib/tests/blackbox/test_cat.py	2011-12-14 12:15:44 +0000
+++ bzrlib/tests/blackbox/test_cat.py	2011-12-14 21:28:31 +0000
@@ -239,6 +239,5 @@
         # 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(16, self.hpss_calls)
-        self.expectFailure("cat still uses VFS calls",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(9, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_checkout.py'
--- bzrlib/tests/blackbox/test_checkout.py	2011-12-14 18:17:43 +0000
+++ bzrlib/tests/blackbox/test_checkout.py	2011-12-14 21:28:31 +0000
@@ -179,8 +179,7 @@
         for count in range(9):
             t.commit(message='commit %d' % count)
         self.reset_smart_call_log()
-        out, err = self.run_bzr(['checkout', self.get_url('from'),
-            'target'])
+        out, err = self.run_bzr(['checkout', self.get_url('from'), 'target'])
         # This figure represent the amount of work to perform this use case. It
         # is entirely ok to reduce this number if a test fails due to rpc_count
         # being too low. If rpc_count increases, more network roundtrips have
@@ -202,9 +201,5 @@
         # 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.
-        if len(self.hpss_calls) < 28 or len(self.hpss_calls) > 40:
-            self.fail(
-                "Incorrect length: wanted between 28 and 40, got %d for %r" % (
-                    len(self.hpss_calls), self.hpss_calls))
-        self.expectFailure("lightweight checkouts require VFS calls",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(15, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_export.py'
--- bzrlib/tests/blackbox/test_export.py	2011-12-14 12:15:44 +0000
+++ bzrlib/tests/blackbox/test_export.py	2011-12-14 21:28:31 +0000
@@ -448,6 +448,5 @@
         # 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(16, self.hpss_calls)
-        self.expectFailure("export requires inventory access which requires VFS",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(7, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_log.py'
--- bzrlib/tests/blackbox/test_log.py	2011-12-14 12:15:44 +0000
+++ bzrlib/tests/blackbox/test_log.py	2011-12-14 21:28:31 +0000
@@ -1086,9 +1086,8 @@
         # 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(19, self.hpss_calls)
-        self.expectFailure("verbose log accesses inventories, which require VFS",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(11, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
 
     def test_per_file(self):
         self.setup_smart_server_with_call_log()
@@ -1103,6 +1102,5 @@
         # 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(21, self.hpss_calls)
-        self.expectFailure("per-file graph access requires VFS",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(15, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_ls.py'
--- bzrlib/tests/blackbox/test_ls.py	2011-12-14 12:15:44 +0000
+++ bzrlib/tests/blackbox/test_ls.py	2011-12-14 21:28:31 +0000
@@ -262,6 +262,5 @@
         # 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(15, self.hpss_calls)
-        self.expectFailure("inventories can only be accessed over VFS",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(6, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/blackbox/test_sign_my_commits.py'
--- bzrlib/tests/blackbox/test_sign_my_commits.py	2011-12-14 18:17:43 +0000
+++ bzrlib/tests/blackbox/test_sign_my_commits.py	2011-12-14 21:28:31 +0000
@@ -165,9 +165,8 @@
         # 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(51, self.hpss_calls)
-        self.expectFailure("signing commits requires VFS access",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(15, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)
 
     def test_verify_commits(self):
         self.setup_smart_server_with_call_log()
@@ -185,12 +184,5 @@
         # 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.
-
-        # The number of readv requests seems to vary depending on the generated
-        # repository and how well it compresses, so allow for a bit of
-        # variation:
-        if len(self.hpss_calls) not in (18, 19):
-            self.fail("Incorrect length: wanted 18 or 19, got %d for %r" % (
-                len(self.hpss_calls), self.hpss_calls))
-        self.expectFailure("verifying commits requires VFS access",
-            self.assertThat, self.hpss_calls, ContainsNoVfsCalls)
+        self.assertLength(10, self.hpss_calls)
+        self.assertThat(self.hpss_calls, ContainsNoVfsCalls)

=== modified file 'bzrlib/tests/per_interbranch/test_push.py'
--- bzrlib/tests/per_interbranch/test_push.py	2011-10-15 01:09:01 +0000
+++ bzrlib/tests/per_interbranch/test_push.py	2011-12-14 21:28:31 +0000
@@ -279,10 +279,10 @@
         # remote graph any further.
         bzr_core_trace = Equals(
             ['Repository.insert_stream_1.19', 'Repository.insert_stream_1.19',
-             'get', 'Branch.set_last_revision_info', 'Branch.unlock'])
+             'Branch.set_last_revision_info', 'Branch.unlock'])
         bzr_loom_trace = Equals(
             ['Repository.insert_stream_1.19', 'Repository.insert_stream_1.19',
-             'get', 'Branch.set_last_revision_info', 'get', 'Branch.unlock'])
+             'Branch.set_last_revision_info', 'get', 'Branch.unlock'])
         self.assertThat(calls_after_insert_stream,
             MatchesAny(bzr_core_trace, bzr_loom_trace))
 

=== modified file 'bzrlib/tests/per_repository_chk/test_supported.py'
--- bzrlib/tests/per_repository_chk/test_supported.py	2009-09-09 18:52:56 +0000
+++ bzrlib/tests/per_repository_chk/test_supported.py	2011-12-14 21:28:31 +0000
@@ -22,7 +22,9 @@
     osutils,
     repository,
     )
+from bzrlib.remote import RemoteRepository
 from bzrlib.versionedfile import VersionedFiles
+from bzrlib.tests import TestNotApplicable
 from bzrlib.tests.per_repository_chk import TestCaseWithRepositoryCHK
 
 
@@ -221,7 +223,9 @@
         # check our setup: B-id and C-id should have identical chk root keys.
         inv_b = b.repository.get_inventory('B-id')
         inv_c = b.repository.get_inventory('C-id')
-        self.assertEqual(inv_b.id_to_entry.key(), inv_c.id_to_entry.key())
+        if not isinstance(repo, RemoteRepository):
+            # Remote repositories always return plain inventories
+            self.assertEqual(inv_b.id_to_entry.key(), inv_c.id_to_entry.key())
         # Now, manually insert objects for a stacked repo with only revision
         # C-id:
         # We need ('revisions', 'C-id'), ('inventories', 'C-id'),
@@ -250,6 +254,9 @@
         for a parent inventory of a new revision is missing.
         """
         repo = self.make_repository('damaged-repo')
+        if isinstance(repo, RemoteRepository):
+            raise TestNotApplicable(
+                "Unable to obtain CHKInventory from remote repo")
         b = self.make_branch_with_multiple_chk_nodes()
         src_repo = b.repository
         src_repo.lock_read()
@@ -293,6 +300,9 @@
         for a parent inventory of a new revision is missing.
         """
         repo = self.make_repository('damaged-repo')
+        if isinstance(repo, RemoteRepository):
+            raise TestNotApplicable(
+                "Unable to obtain CHKInventory from remote repo")
         b = self.make_branch_with_multiple_chk_nodes()
         b.lock_read()
         self.addCleanup(b.unlock)

=== modified file 'bzrlib/tests/per_repository_vf/test_add_inventory_by_delta.py'
--- bzrlib/tests/per_repository_vf/test_add_inventory_by_delta.py	2011-05-02 22:39:15 +0000
+++ bzrlib/tests/per_repository_vf/test_add_inventory_by_delta.py	2011-12-14 21:28:31 +0000
@@ -109,4 +109,4 @@
         else:
             repo_delta.commit_write_group()
         self.assertEqual(add_validator, delta_validator)
-        self.assertEqual(new_inv, inv)
+        self.assertEqual(list(new_inv.iter_entries()), list(inv.iter_entries()))

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2011-12-14 12:20:36 +0000
+++ bzrlib/tests/test_remote.py	2011-12-14 21:28:31 +0000
@@ -69,6 +69,7 @@
 from bzrlib.smart.repository import (
     SmartServerRepositoryGetParentMap,
     SmartServerRepositoryGetStream_1_19,
+    _stream_to_byte_stream,
     )
 from bzrlib.symbol_versioning import deprecated_in
 from bzrlib.tests import (
@@ -4233,3 +4234,43 @@
             'Repository.unlock', ('quack/', 'token', 'False'),
             'success', ('ok', ))
         repo.pack(['hinta', 'hintb'])
+
+
+class TestRepositoryIterInventories(TestRemoteRepository):
+    """Test Repository.iter_inventories."""
+
+    def _serialize_inv_delta(self, old_name, new_name, delta):
+        serializer = inventory_delta.InventoryDeltaSerializer(True, False)
+        return "".join(serializer.delta_to_lines(old_name, new_name, delta))
+
+    def test_single_empty(self):
+        transport_path = 'quack'
+        repo, client = self.setup_fake_client_and_repository(transport_path)
+        fmt = bzrdir.format_registry.get('2a')().repository_format
+        repo._format = fmt
+        stream = [('inventory-deltas', [
+            versionedfile.FulltextContentFactory('somerevid', None, None,
+                self._serialize_inv_delta('null:', 'somerevid', []))])]
+        client.add_expected_call(
+            'VersionedFileRepository.get_inventories', ('quack/', 'unordered'),
+            'success', ('ok', ),
+            _stream_to_byte_stream(stream, fmt))
+        ret = list(repo.iter_inventories(["somerevid"]))
+        self.assertLength(1, ret)
+        inv = ret[0]
+        self.assertEquals("somerevid", inv.revision_id)
+
+    def test_empty(self):
+        transport_path = 'quack'
+        repo, client = self.setup_fake_client_and_repository(transport_path)
+        ret = list(repo.iter_inventories([]))
+        self.assertEquals(ret, [])
+
+    def test_missing(self):
+        transport_path = 'quack'
+        repo, client = self.setup_fake_client_and_repository(transport_path)
+        client.add_expected_call(
+            'VersionedFileRepository.get_inventories', ('quack/', 'unordered'),
+            'success', ('ok', ), iter([]))
+        self.assertRaises(errors.NoSuchRevision, list, repo.iter_inventories(
+            ["somerevid"]))

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2011-12-14 12:20:36 +0000
+++ bzrlib/tests/test_smart.py	2011-12-14 21:28:31 +0000
@@ -32,6 +32,7 @@
     bzrdir,
     errors,
     gpg,
+    inventory_delta,
     tests,
     transport,
     urlutils,
@@ -2565,6 +2566,8 @@
             smart_repo.SmartServerRepositoryAbortWriteGroup)
         self.assertHandlerEqual('VersionedFileRepository.get_serializer_format',
             smart_repo.SmartServerRepositoryGetSerializerFormat)
+        self.assertHandlerEqual('VersionedFileRepository.get_inventories',
+            smart_repo.SmartServerRepositoryGetInventories)
         self.assertHandlerEqual('Transport.is_readonly',
             smart_req.SmartServerIsReadonly)
 
@@ -2630,3 +2633,48 @@
             smart_req.SuccessfulSmartServerResponse(('ok', ), ),
             request.do_body(''))
 
+
+class TestSmartServerRepositoryGetInventories(tests.TestCaseWithTransport):
+
+    def _get_serialized_inventory_delta(self, repository, base_revid, revid):
+        base_inv = repository.revision_tree(base_revid).inventory
+        inv = repository.revision_tree(revid).inventory
+        inv_delta = inv._make_delta(base_inv)
+        serializer = inventory_delta.InventoryDeltaSerializer(True, False)
+        return "".join(serializer.delta_to_lines(base_revid, revid, inv_delta))
+
+    def test_single(self):
+        backing = self.get_transport()
+        request = smart_repo.SmartServerRepositoryGetInventories(backing)
+        t = self.make_branch_and_tree('.', format='2a')
+        self.addCleanup(t.lock_write().unlock)
+        self.build_tree_contents([("file", "somecontents")])
+        t.add(["file"], ["thefileid"])
+        t.commit(rev_id='somerev', message="add file")
+        self.assertIs(None, request.execute('', 'unordered'))
+        response = request.do_body("somerev\n")
+        self.assertTrue(response.is_successful())
+        self.assertEquals(response.args, ("ok", ))
+        stream = [('inventory-deltas', [
+            versionedfile.FulltextContentFactory('somerev', None, None,
+                self._get_serialized_inventory_delta(
+                    t.branch.repository, 'null:', 'somerev'))])]
+        fmt = bzrdir.format_registry.get('2a')().repository_format
+        self.assertEquals(
+            "".join(response.body_stream),
+            "".join(smart_repo._stream_to_byte_stream(stream, fmt)))
+
+    def test_empty(self):
+        backing = self.get_transport()
+        request = smart_repo.SmartServerRepositoryGetInventories(backing)
+        t = self.make_branch_and_tree('.', format='2a')
+        self.addCleanup(t.lock_write().unlock)
+        self.build_tree_contents([("file", "somecontents")])
+        t.add(["file"], ["thefileid"])
+        t.commit(rev_id='somerev', message="add file")
+        self.assertIs(None, request.execute('', 'unordered'))
+        response = request.do_body("")
+        self.assertTrue(response.is_successful())
+        self.assertEquals(response.args, ("ok", ))
+        self.assertEquals("".join(response.body_stream),
+            "Bazaar pack format 1 (introduced in 0.18)\nB54\n\nBazaar repository format 2a (needs bzr 1.16 or later)\nE")

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-12-14 20:39:58 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-12-14 21:28:31 +0000
@@ -95,6 +95,10 @@
 
 * New HPSS call ``BzrDir.checkout_metadir``. (Jelmer Vernooij, #894459)
 
+* New HPSS call ``VersionedFileRepository.get_inventories``,
+  speeding up various commands including ``bzr export``,
+  ``bzr checkout`` and ``bzr cat``. (Jelmer Vernooij, #608640)
+
 Testing
 *******
 
@@ -156,6 +160,9 @@
 * Plugins can now register additional "location aliases".
   (Jelmer Vernooij)
 
+* ``bzr status`` no longer shows shelves if files are specified.
+  (Francis Devereux)
+
 * Revision specifiers will now only browse as much history as they
   need to, rather than grabbing the whole history unnecessarily in some
   cases. (Jelmer Vernooij)
@@ -281,8 +288,14 @@
   ``Repository.get_revision_signature_text``.
   (Jelmer Vernooij)
 
+* Add HPSS call for ``Branch.get_checkout_format``. (Jelmer Vernooij, #894459)
+
 * Add HPSS call for ``Repository.pack``. (Jelmer Vernooij, #894461)
 
+* Add HPSS calls for ``Repository.iter_files_bytes``, speeding up
+  several commands including ``bzr export`` and ``bzr co --lightweight``.
+  (Jelmer Vernooij, #608640)
+
 * Custom HPSS error handlers can now be installed in the smart server client
   using the ``error_translators`` and ``no_context_error_translators``
   registries. (Jelmer Vernooij)