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

John A Meinel (jameinel) wrote :

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.

Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla -


« Back to merge proposal