Bazaar

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

faster-revert-593560
Merge into bzr.dev

Proposed by Andrew Bennetts on 2010-08-30

Status:

Merged

Approved by:

Andrew Bennetts on 2010-09-01

Approved revision:

no longer in the source branch.

Merged at revision:

5401

Proposed branch:

lp://staging/~spiv/bzr/faster-revert-593560

Merge into:

lp://staging/bzr

Diff against target:

56 lines (+11/-3)

3 files modified

NEWS (+4/-0)
bzrlib/tree.py (+1/-1)
doc/en/whats-new/whats-new-in-2.3.txt (+6/-2)

To merge this branch:

bzr merge lp://staging/~spiv/bzr/faster-revert-593560

High

Confirmed

Link a bug report

Reviewer	Review Type	Date Requested	Status
bzr-core		2010-08-30	Pending
Review via email: mp+34043@code.staging.launchpad.net

Commit message

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

Description of the change

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.

Revision history for this message

Martin Pool (mbp) wrote on 2010-08-30:

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

Revision history for this message

John A Meinel (jameinel) wrote on 2010-08-30:

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

Revision history for this message

Martin Pool (mbp) wrote on 2010-08-31:

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

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-09-01:

sent to pqm by email