Merge lp:~spiv/bzr/faster-revert-593560 into lp:bzr

[Andrew Bennetts]

Avoid repeatedly calling self.target.all_file_ids() in InterTree.iter_changes.

This simple change improves bzr revert and bzr status on large trees (i.e. gcc-linaro) with many changes (i.e. reverting from lp:gcc-linaro/4.5 to tag:gcc_4_5_0_release) by about 15% on my system. Specifically bzr revert to that tag goes from 3m 49s to 3m 18s, and bzr st on the resulting tree also drops from about 1.8s to 1.6s.

Tests still pass, and avoiding unnecessary (and repeated) calls to all_file_ids seems like a clearly good idea.

[Martin Pool (mbp)]

That's pretty nice.

Perhaps the news entry can say a bit more about what changed so we can
understand it later: "by not repeatedly building a list of file ids."

I'd like you to add this to the whatsnew-2.3 docs too, under a
performance heading.

In some ways this patch feels incomplete not to have a test or a
prevention of regressions. I guess it is reasonable to keep
all_file_ids for people who want it. But perhaps just measuring
performance is the best way to keep it up, so +1 with those doc
tweaks.

--
Martin

[John A Meinel (jameinel)]

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

On 8/30/2010 3:55 AM, Martin Pool wrote:
> That's pretty nice.
>
> Perhaps the news entry can say a bit more about what changed so we can
> understand it later: "by not repeatedly building a list of file ids."
>
> I'd like you to add this to the whatsnew-2.3 docs too, under a
> performance heading.
>
> In some ways this patch feels incomplete not to have a test or a
> prevention of regressions. I guess it is reasonable to keep
> all_file_ids for people who want it. But perhaps just measuring
> performance is the best way to keep it up, so +1 with those doc
> tweaks.
>

Can you switch it to use 'target.has_id()' instead? I think Martin has
mentioned that he doesn't really prefer the '__contains__' syntax for
trees. Certainly it is a bit unclear what 'in tree' should be passed.
(It takes a file_id, but could be a InventoryEntry, etc.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkx7upQACgkQJdeBCYSNAAOmUwCeOvR1CsV04QEq7gaS4co/xc4X
xEcAoL5OwV2iJLq1oqrj5DCcCCflg5gt
=LQSN
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

On 31 August 2010 00:05, John Arbash Meinel <email address hidden> wrote:
> Can you switch it to use 'target.has_id()' instead? I think Martin has
> mentioned that he doesn't really prefer the '__contains__' syntax for
> trees. Certainly it is a bit unclear what 'in tree' should be passed.
> (It takes a file_id, but could be a InventoryEntry, etc.)

Yes, if we have that and can use it I think it is much more explicit.

--
Martin

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-08-30 10:14:21 +0000
+++ NEWS	2010-08-31 07:14:41 +0000
@@ -149,6 +149,10 @@
   will be backed up (adding an extention of the form .~#~).
   (Marius Kruger, #400554)
 
+* ``bzr revert`` and ``bzr status`` are up to 15% faster on large trees
+  with many changes by not repeatedly building a list of all file-ids.
+  (Andrew Bennetts)
+
 * Decrease memory consumption when many chk index pages are loaded. (Such
   as during ``bzr co`` or ``bzr ls -R`` of a large tree.) Often we need to
   read many chk pages because the individual chk map nodes will be spread

=== modified file 'bzrlib/tree.py'
--- bzrlib/tree.py	2010-05-25 17:27:52 +0000
+++ bzrlib/tree.py	2010-08-31 07:14:41 +0000
@@ -1130,7 +1130,7 @@
             if file_id in to_paths:
                 # already returned
                 continue
-            if file_id not in self.target.all_file_ids():
+            if not self.target.has_id(file_id):
                 # common case - paths we have not emitted are not present in
                 # target.
                 to_path = None

=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
--- doc/en/whats-new/whats-new-in-2.3.txt	2010-08-23 18:59:22 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-08-31 07:14:41 +0000
@@ -8,12 +8,12 @@
 :doc:`../release-notes/index` for a full list.
 
 Users are encouraged to upgrade from the other stable series.  This
-document outlines the improvements in Bazaar 2.2 vs Bazaar 2.1. As well as
+document outlines the improvements in Bazaar 2.3 vs Bazaar 2.2. As well as
 summarizing improvements made to the core product, it highlights
 enhancements within the broader Bazaar world of potential interest to
 those upgrading.
 
-Bazaar 2.2.0 is fully compatible both locally and on the network with 2.0
+Bazaar 2.3.0 is fully compatible both locally and on the network with 2.0
 2.1, and 2.2, and can read and write repositories generated by all
 previous versions.
 
@@ -28,6 +28,10 @@
 Performance improvements
 ************************
 
+* ``bzr revert`` and ``bzr status`` are up to 15% faster on large trees
+  with many changes by not repeatedly building a list of all file-ids.
+  (Andrew Bennetts)
+
 * ``bzr send`` uses less memory.  
   (John Arbash Meinel, #614576)