Merge into bzr.dev : hpss-get-inventories : Code : Bazaar

Reviewer	Review Type	Date Requested	Status
Vincent Ladeuil		2011-11-29	Approve on 2011-12-02
Review via email: mp+83837@code.staging.launchpad.net

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-11-29:

#

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

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-11-29:

#

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.

Revision history for this message

Andrew Bennetts (spiv) wrote on 2011-11-29:

#

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.

Revision history for this message

Martin Pool (mbp) wrote on 2011-11-30:

#

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

Revision history for this message

Vincent Ladeuil (vila) wrote on 2011-12-02:

#

>> 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

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-12-03:

#

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

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

Revision history for this message

Andrew Bennetts (spiv) wrote on 2011-12-05:

#

Download full text (4.5 KiB)

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 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 data from a repository.”  The existing
get_record_stream stuff is pretty complex, but IIRC that's from two sources:

1. the complexity of constructing a definitely
    ready-to-commit-perhaps-after-a-single-fixup-fetch-for-stacking-invariants
    stream for the “give me all revisions/invs/texts/etc records for revisions
    in this subgraph of the revision graph”.  That's mostly independent of
    get_record_stream, it just happens to be where the logic for that is
    encoded.

2. the abstractions/indirections to allow various different repository formats
    to send their own native records directly via record streams.

I think 2 is a strong reason to keep using records streams, rather than
inventing something new and discovering that out of necessity you have ended up
reinventing it.

The basic structure of a record stream itself is very simple and lightweight
(although it may well benefit from being wrapped in zlib compression over the
wire?), and should be reused as the default decision for repository data unless
there's a really good reason not to.

The current method of extending get_record_stream by shoehorning more and more
query types into the one kind of call perhaps isn't a good tradeoff.  I think at
some level it will boil down to the same work though, even if the veneer is
get_inventories(invs) rather than get_record_stream(JustInventories(invs), …).

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2011-12-05:

#

Download full text (6.7 KiB)

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'...

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'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 data from a repository.”  The existing
> get_record_stream stuff is pretty complex, but IIRC that's from two sources:
>
>   1. the complexity of constructing a definitely
>      ready-to-commit-perhaps-after-a-single-fixup-fetch-for-stacking-invariants
>      stream for the “give me all revisions/invs/texts/etc records for revisions
>      in this subgraph of the revision graph”.  That's mostly independent of
>      get_record_stream, it just happens to be where the logic for that is
>      encoded.
>
>   2. the abstractions/indirections to allow various different repository formats
>      to send their own native records directly via record streams.
>
> I think 2 is a strong reason to keep using records streams, rather than
> inventing something new and discovering that out of necessity you have ended up
> reinventing it.
>
> The basic structure of a record stream itself is very simple and lightweight
> (although it may well benefit from being wrapped in zlib compression over the
> wire?), and should be reused as the default decision for repository data unless
> there's a really good reason not to.
>
> The current method of extending get_record_stream by shoehorning more and more
> query types into the one kind of call perhaps isn't a good tradeoff.  I think at
> some level it will boil down to the same work though, even if the veneer is
> get_inventories(invs) rather than get_record_stream(JustInventories(invs), …).
I've done some more digging in the get_record_stream verb code today. 
Using record streams seems reasonable to me, but I would like to use a 
separate verb (rather than Repository.get_stream_1.19 and friends) to 
keep the interface simple - there's so much going on in 
get_record_stream that we don't need here.

It seems like there are two things we can do - either just send a stream 
similar to get_record_stream_1.19 with a single inventory-delta 
"substream", or we could just send the substream with inventory deltas. 
The latter is simpler and would need slightly less bandwidth, but can't 
be used with e.g. insert_record_stream. I'm not sure if we care about 
that, though. What do you think?

The body_stream method of the GetInventories server side implementation 
would look something like this if we were directly using substreams:

def body_stream(self, repository, ordering, revids):
         pack_writer = pack.ContainerSerialiser()
         yield pack_writer.begin()
         serializer = inventory_delta.InventoryDeltaSerializer(
             repository.supports_rich_root(),
             repository._format.supports_tree_reference)

prev_inv = _mod_inventory.Inventory(root_id=None,
             revision_id=_mod_revision.NULL_REVISION)
         self._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 pack_writer.bytes_record("".join(lines), 
[('inventory-delta',)])
                 prev_inv = inv
         finally:
             self._repository.unlock()
         yield pack_writer.end()

Thanks again for reviewing this - I really appreciate your thoughts.

Cheers,

Jelmer

Bazaar

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

Commit message

Description of the change

Preview Diff

Subscribers