[MERGE] Native dirstate update-basis-via-delta
Robert Collins
robertc at robertcollins.net
Wed Oct 24 01:45:08 BST 2007
On Tue, 2007-10-23 at 18:49 -0500, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robert Collins wrote:
> > This enables the delta-to-basis-from-commit that Martin and I
> > collaborated on over the last fortnight.
> >
> > For my tests, 'bzr commit FOO' drops from 15 seconds to 10 on a mozilla
> > tree. Initial commit and regular incremental commit is also faster.
> >
> > -Rob
> >
>
> _discard_merge_parents iterates over all entries and just removes everything
> past the first node.
>
> What if instead it was an iterator itself and would yield all the nodes. It
> just seems a little weird to remove all of the ('a', 'a') entries when you may
> be introducing more. It would also mean that you could do 1 pass instead of
> multiple.
We can't do the change in one pass because of ordering with renames. I
noted that in the delta expansion logic.
> But from a correctness perspective it looks good.
>
>
> You are using "cache_utf8.encode" to convert path names. But they are unlikely
> to actually be repeated. So the caching probably ends up hurting (by bloating
> the cache, and having a key lookup failure for each entry.)
Currently they are repeated (because all the paths have previously been
handed to _get_entry in _sha_from_stat in working tree 4). Possibly with
my other patch I just sent this should be changed.
> You might want to use:
>
> encode = cache_utf8._utf8_encode
>
> path = encode(path)[0]
>
> + # Because renames must preserve their children we must have
> + # processed all reloations and removes before hand. The sort
>
> relocations
Thanks.
> ...
> + self._update_basis_apply_deletes(deletes)
> + deletes = []
>
> ^- In the inner loop you seem to apply all deletes whenever you see a renamed
> file. This seems a little odd to me, though I won't say it is incorrect.
The comment right above it explains why in it's first sentence; if you
could see why it didn't gel for you that would be great.
> ...
> + new_deletes = list(reversed(list(self._iter_child_entries(1,
> + encode(old_path)))))
>
> Does _iter_child_entries return an empty list for a file?
Yes, as per its docstring ;).
> list(reversed(list( seems a little suspect
> Especially since the only thing you do with it is
> for entry in new_deletes
Yah, slipped in when testing.
> So at a minimum
> new_deletes = reversed(list(self._iter_child_entries...)
>
> ...
> + self._update_basis_apply_deletes(deletes)
> + self._update_basis_apply_changes(changes)
> + self._update_basis_apply_adds(adds)
> +
> + # remove all deletes
> + self._dirblock_state = DirState.IN_MEMORY_MODIFIED
> + self._header_state = DirState.IN_MEMORY_MODIFIED
> + return
>
> ^- This comment is misplaced. It either goes before
> _update_basis_apply_deletes, or it should just be removed.
Removed.
>
> Explicitly using:
>
> absent = ('r', 'a')
> ...
> if kind not in absent:
> ...
>
> Is probably slower than either
>
...
> x in 'ar'
>
> is equivalent to
> y = set('ar')
> x in y
>
> Both are around 220 us.
>
> but
> y = ('a', 'r')
>
> If x == 'a' then it is faster (209us), if it is 'r' then it is slower (250us),
> if it is not present it is much slower (278us).
>
> My guess is that "x in 'ar'" is implemented as a set check. And that "x in
> ('a', 'r')" is implemented as a comparison against each entry in the tuple.
I doubt that 'x in "ar"' is a set check, rather its probably special
cased - IIRC there are even CPU specific instructions that can perform
this. Changing to use strings though.
I wonder if for
'aa' in 'aa ar ra rr'
is better than
'aa' in set('aa', 'ar', 'ra, 'rr')
(obviously with the construction outside the loop).
> ...
> + if kind == 'd':
> + next_pending_dirs.append('/'.join(entry[0][0:2]))
>
> I'm pretty sure that
> entry[0][0] + '/' + entry[0][1]
> is faster than the '/'.join statement. Because the join has to create a new
> 2-entry list, as well as build up the string.
Will change.
> ...
>
> + def create_dirstate_with_two_trees(self):
> + """This dirstate contains multiple files and directories.
>
> ^- You directly create the dirblock entries. This seems more error prone than
> doing it through a Tree interface. I won't say it is wrong, just that small
> errors are a lot harder to detect.
I was using the style other tests here used.
> Overall, I don't see anything obviously wrong :). I would say that I would like
> to see a whole lot more tests.
Any specific area? We have a number of tests of this already -
wt_implementations.get_parents tests just about every permutation I
could think of.
> One problem we have with dirstate, is that if the basis inventory gets skewed
> from reality, it can do nasty things, and we have very limited ability to
> detect it.
>
> Probably 'bzr check' should assert that "tree.basis_tree()" gives the same
> result at "tree.branch.repository.revision_tree(tree.last_revision())". That
> would at least go a way towards giving security in something like this which is
> mutating over time. (There is no point where we forcibly "sync" and verify that
> everything is 100% correct.)
I think thats a good idea. Also bzr check should call tree._validate().
Do you want these done as part of this patch?
Also, do you think the _id_index should be adjusted/reset by this? I
think resetting it is ok and sufficient.
-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071024/64196a74/attachment.pgp
More information about the bazaar
mailing list