Code review comment for lp:~spiv/bzr/inventory-delta

On Thu, 2009-08-06 at 16:15 +0000, John A Meinel wrote:
>
> if utf8_path.startswith('/'):
>
> ^- If this is a core routine (something called for every path) then:
> if utf8_path[:1] == '/':
>
> *is* faster than .startswith() because you
> 1) Don't have a function call
> 2) Don't have an attribute lookup
>
> I'm assuming this is a function that gets called a lot. If not, don't
> worry about it.

utf8_path[:1] == '/' requires a string copy though, for all that its
heavily tuned in the VM.

> ...
>
> 566 + if required_version < (1, 18):
> 567 + # Remote side doesn't support inventory deltas. Wrap the
> stream to
> 568 + # make sure we don't send any. If the stream contains
> inventory
> 569 + # deltas we'll interrupt the smart insert_stream request and
> 570 + # fallback to VFS.
> 571 + stream = self._stop_stream_if_inventory_delta(stream)
>
> ^- it seems a bit of a shame that if we don't support deltas we fall
> back to
> VFS completely, rather than trying something intermediate (like
> falling back to
> the original code path of sending full inventory texts, or IDS, or...)
>
> I think we are probably okay, but this code at least raises a flag. I
> expect a
> bug report along the lines of "fetching between 1.18 and older server
> is very
> slow". I haven't looked at all the code paths to determine if 1.18
> will have
> regressed against a 1.17 server. Especially when *not* converting
> formats. Have
> you at least manually tested this?

We don't want to require rewindable streams; falling back to VFS is by
far the cleanest way to fallback without restarting the stream or
requiring rewinding. I agree that there is a risk of performance issues,
OTOH launchpad, our largest deployment, will upgrade quickly :).
...
> ^- I think this should probably be ".network_name() ==
> other.network_name()"
> and we just customize the names to be the same. Is that possible to
> do?

It would be a little difficult with clients deployed already that don't
know the names are the same, and further, it would make it impossible to
request initialisation of one of them. In my review I' suggesting just
serializer equality is all thats needed.

Anyhow, my review is about half done, getting back to it ones the mail
surge is complete.

-Rob