Bazaar

Code review comment for lp:~ian-clatworthy/bzr/faster-dirstate-saving

faster-dirstate-saving
Merge into trunk-old

Revision history for this message

John A Meinel (jameinel) wrote on 2009-06-05:

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

Andrew Bennetts wrote:
> Review: Approve
> I'm no dirstate expert, so depending on how confident you are with this patch you might like to wait for another review before merging.
>
> Overall, I this change seems ok to me. It deserves careful benchmarking of course, but I know you've been doing that :)
>
> It's interesting to me that _build_line_cache actually re-reads the lines from the file.

I'm a bit surprised that re-reading the file is significantly faster
than converting a tuple of strings back into a serialized string. If it
is significant, than I'd certainly rather a pyrex extension than reading
from disk...

>
> + :param worth_saving_limit: when the exact number of changed entries
> + is known, only bother saving the dirstate if more than this
> + count of entries have changed. -1 means never save.
>
> I'd personally prefer "None means never save", but that's not a big deal.
>
> I believe the dirstate can record changes to tree state, not just cache stat info...
> so is there a risk with this patch that e.g. "bzr mv foo bar" will go
unrecorded because it's under the _worth_saving_limit? I understand the
value in not updating the cache for only a small number of changes, but
AFAIK we always want to update the dirstate file when actual state is
changed. I think the header_too=True flag is used to make sure this
works, but that seems like a pretty indirect way of expressing the
concept that this is a non-optional update.

The *big* think to watch out for is that there is "IN_MEMORY_MODIFIED"
that happens from a stat cache update, which can be trivially ignored.
And there is "IN_MEMORY_MODIFIED" that happens from "bzr add" which
*must* be written out.

I didn't look at the code to see if Ian clearly distinguished them, if
he has, then we are fine.

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

iEYEARECAAYFAkoogDcACgkQJdeBCYSNAAPFWQCfeAnQLM2UPq6K+BVsleEy+szw
UIsAoKeT7L7Jybvz8vHUZI32kw9XSmNP
=SWQG
-----END PGP SIGNATURE-----

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

Andrew Bennetts wrote:
> Review: Approve
> I'm no dirstate expert, so depending on how confident you are with this patch you might like to wait for another review before merging.
> 
> Overall, I this change seems ok to me.  It deserves careful benchmarking of course, but I know you've been doing that :)
> 
> It's interesting to me that _build_line_cache actually re-reads the lines from the file.

> 
> +        :param worth_saving_limit: when the exact number of changed entries
> +            is known, only bother saving the dirstate if more than this
> +            count of entries have changed. -1 means never save.
> 
> I'd personally prefer "None means never save", but that's not a big deal.
> 
> I believe the dirstate can record changes to tree state, not just cache stat info...
> so is there a risk with this patch that e.g. "bzr mv foo bar" will go
unrecorded because it's under the _worth_saving_limit?  I understand the
value in not updating the cache for only a small number of changes, but
AFAIK we always want to update the dirstate file when actual state is
changed.  I think the header_too=True flag is used to make sure this
works, but that seems like a pretty indirect way of expressing the
concept that this is a non-optional update.

I didn't look at the code to see if Ian clearly distinguished them, if
he has, then we are fine.

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

iEYEARECAAYFAkoogDcACgkQJdeBCYSNAAPFWQCfeAnQLM2UPq6K+BVsleEy+szw
UIsAoKeT7L7Jybvz8vHUZI32kw9XSmNP
=SWQG
-----END PGP SIGNATURE-----

« Back to merge proposal