Code review comment for lp:~jameinel/bzr/2.0.1-faster-log-dir-bug374730

On Thu, 2009-09-24 at 20:45 +0000, John A Meinel wrote:
> John A Meinel has proposed merging
> lp:~jameinel/bzr/2.0.1-faster-log-dir-bug374730 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #374730 log dir is slow in development-rich-root
> https://bugs.launchpad.net/bugs/374730
>
>
> This is my 'phase 1' for making 'bzr log DIR' faster.
>
> It implements a custom 'CHKInventory.filter()' which only has to deal
> in the actual files that are requested, rather than always doing
> 'iter_entries()' and walking the whole tree.
>
> This makes 'bzr log DIR' scale based on the number of entries in DIR.
> In my testing,
>
> 5m30s bzr.dev log tools
> 5m30s newbzr log bzrlib
> 0m36s newbzr log tools
>
> So when the number of files is low, the time drops from 5m30s => 36s,
> which is great.
>
> The next step is probably going to bypass 'filter()' for 'log DIR' and
> solely layer on top of 'iter_changes'. My initial (broken) testing
> showed that it stayed consistently around 36s for both bzrlib and
> tools, which is certainly a lot better.
>
> I'd like to land this in the 2.0.x series, because it seems like a
> simple bug fix.
>
> The size of the delta is a bit large, because it turned out that
> CHKInventory.filter() wasn't tested at all. (See my mailing list
> thread about what 'per_inventory' is actually supposed to be testing.)
> I went ahead and made 'per_inventory' actually care about both
> inventory implementations, and moved the tests around accordingly.

So this looks broadly fine; we didn't do much more than tune log for the
O(tree) datastore we had previously.

See my list response for thoughts on the test layout [short story -
test_inv.py is where I would have added tests].

 review +1