Merge lp:~jameinel/bzr/2.1.0rc1-set-mtime into lp:bzr
- 2.1.0rc1-set-mtime
- Merge into bzr.dev
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~jameinel/bzr/2.1.0rc1-set-mtime |
Merge into: | lp:bzr |
Diff against target: |
108 lines (+43/-1) 3 files modified
NEWS (+6/-0) bzrlib/tests/test_transform.py (+25/-1) bzrlib/transform.py (+12/-0) |
To merge this branch: | bzr merge lp:~jameinel/bzr/2.1.0rc1-set-mtime |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Approve | ||
Martin Packman (community) | Approve | ||
Review via email: mp+16881@code.launchpad.net |
Commit message
Description of the change
John A Meinel (jameinel) wrote : | # |
Robert Collins (lifeless) wrote : | # |
On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
>
> Anyway, it doesn't seem like a huge overhead, as long as it does help
> people.
Scott reported this issue to me - Scott, can you try pulling this branch
and seeing if it helps?
https:/
-Rob
Martin Packman (gz) wrote : | # |
Can you make the api take a file object instead? (In other words, call f.fileno() in the extension function). That would then mean I can implement it on non-C based pythons.
Is there some reason os.utime can't be used?
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin [gz] wrote:
> Review: Needs Information
> Can you make the api take a file object instead? (In other words, call f.fileno() in the extension function). That would then mean I can implement it on non-C based pythons.
>
> Is there some reason os.utime can't be used?
#1, I didn't find it while looking around.
#2, it works solely on paths, rather than the open file handle, but I'm
happy to use it as a fallback.
#3 The reason to use use a fileno is because both code paths needed that
anyway, and I thought you might have a file opened by "os.open()" rather
than "open()". However, if it makes it better for other systems, I can
certainly go back to passing the file object itself.
I suppose one concern is that people may pass "StringIO()" etc to the
function, but it doesn't really matter.
I'll update accordingly so you can get another look at it before it gets
merged. Thanks for the pointer to os.utime. I swear I went through the
"os" documentation quite thoroughly and didn't find it. But I'll admit
to looking for one that took a file object/file descriptor and not just
a path.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
NfoAn30jWbmd9C3
=P8ov
-----END PGP SIGNATURE-----
Martin Packman (gz) wrote : | # |
> > Is there some reason os.utime can't be used?
>
> #1, I didn't find it while looking around.
Thought that was perhaps the case, the new python documentation with everything about a module on one page does make it easier to get lost.
> #2, it works solely on paths, rather than the open file handle, but I'm
> happy to use it as a fallback.
It may well have more overhead, so something like this in the compiled extensions could still be worthwhile.
> I suppose one concern is that people may pass "StringIO()" etc to the
> function, but it doesn't really matter.
Pyrex can throw attribute errors if obj.fileno doesn't exist, right?
Change looks good to me now, a few teeny comments on the updated diff:
+ systems to achieve this effect. This only works when extensions are
+ compiled as pure-python does not expose similar functionality.
NEWS needs updating.
+import msvcrt
...
+ the_handle = <HANDLE>
I don't understand Pyrex completely, is the import needed to access the C library? If so, why no "msvcrt." prefix?
+_set_mtime_func = None
+def fset_mtime(f, mtime):
There are too many ways of lazily using the right function for the platform in this file. With this way, isn't the None check in the body unneeded as it'll always (but only on the first call) be None?
+ This uses native OS functionality to set file times. As such, if extensions
+ are not compiled, this function becomes a no-op.
Docstring wants updating.
+ try:
+ from bzrlib.
Could rearrange this body a little to have only one try-block and no else clauses if the platform check was inside the try and the imports used `... as _set_mtime_func` - depends what you think is clearer.
+# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
It's 2010 now!
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
> + systems to achieve this effect. This only works when extensions are
> + compiled as pure-python does not expose similar functionality.
>
> NEWS needs updating.
yep, thanks for noticing.
>
> +import msvcrt
> ...
> + the_handle = <HANDLE>
>
> I don't understand Pyrex completely, is the import needed to access the C library? If so, why no "msvcrt." prefix?
I don't need to import msvcrt (and have thus removed the import). This
is the Win32 api function I'm accessing at this point. I just started
with msvcrt.
>
> +_set_mtime_func = None
> +def fset_mtime(f, mtime):
>
> There are too many ways of lazily using the right function for the platform in this file. With this way, isn't the None check in the body unneeded as it'll always (but only on the first call) be None?
>
I don't quite understand the issue here. On the first call to
fset_mtime() it will pick the correct implementation to use, set the
global variable, and then use it from then on. The None check is
explicitly needed to determine whether the check has been done before.
I really wanted this to be lazy, because otherwise you have circular
dependency issues and other weird results if you import extension code
in osutils. For example "_walkdirs_
'setup.py' which rebuilds extensions imports osutils for some
functionality. So if you have a non-lazy implementation, then it ends up
unable to rebuild the file (because it is already open).
> + This uses native OS functionality to set file times. As such, if extensions
> + are not compiled, this function becomes a no-op.
>
> Docstring wants updating.
>
> + try:
> + from bzrlib.
>
> Could rearrange this body a little to have only one try-block and no else clauses if the platform check was inside the try and the imports used `... as _set_mtime_func` - depends what you think is clearer.
>
It is shorter, not sure if it is much clearer.
> +# Copyright (C) 2006, 2007, 2008, 2009 Canonical Ltd
>
> It's 2010 now!
Unfortunately, after 2012 we'll run out of space to keep this under 80
chars. I forget why we aren't supposed to use ranges, and wonder if we
should just switch anyway.
Thanks for the review. Update pushed.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
ZO4AoLHfP44B5Kt
=K4aB
-----END PGP SIGNATURE-----
Martin Packman (gz) wrote : | # |
Ack, okay, I've tested this here and there are some outstanding issues I didn't see eyeballing the code. I'll get them myself and push a branch rather than making you do more work though.
* The tests for "same second" need to use round() rather than int()
* GetFileInformat
I'll replace that with SetFileTime, shout if you can think of a reason not to.
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin [gz] wrote:
> Ack, okay, I've tested this here and there are some outstanding issues I didn't see eyeballing the code. I'll get them myself and push a branch rather than making you do more work though.
>
> * The tests for "same second" need to use round() rather than int()
> * GetFileInformat
>
> I'll replace that with SetFileTime, shout if you can think of a reason not to.
I'm fine with SetFileTime. I new that "Ex" was newer, but I didn't see
the specific version. SetFileTime is even 2k+, which potentially means
it won't work for 95/98. But I don't think the extension works there anyway.
GetInformationB
don't even need it for our case. Namely, I only wanted to change mtime
(LastWriteTime) and leave access time, etc alone. But SetFileTime lets
you pass NULL for those values.
As for "round()" versus "int()". I can guarantee that int() was
necessary on the Linux machine I tested on that had 1s precision. I
haven't tested on a FAT system for Windows, or anything other than
Vista. And Vista worked just fine without round *or* int. (The 1ms
resolution of time.time() was easily round-tripped to the filesystem.)
So I would probably suggest using "assertAlmostEq
1s resolution. The important part is that 2 files end up with the exact
same time, and the time shouldn't be before the time supplied. (To make
sure that people doing sub-second reverts get timestamps that are new
enough.)
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
rf4An0N4ihvjkUJ
=lcqL
-----END PGP SIGNATURE-----
Robert Collins (lifeless) wrote : | # |
On Wed, 2010-01-06 at 19:03 +0000, John A Meinel wrote:
> he important part is that 2 files end up with the exact
> same time, and the time shouldn't be before the time supplied. (To
> make
> sure that people doing sub-second reverts get timestamps that are new
> enough.)
How do you deal with FAT's 2 second window then? Surely it will go
backwards sometimes (to the start of the 2 seconds). Or is the time
supplied the time of one of the reverted files?
--Rob
Martin Packman (gz) wrote : | # |
> How do you deal with FAT's 2 second window then? Surely it will go
> backwards sometimes (to the start of the 2 seconds). Or is the time
> supplied the time of one of the reverted files?
This was in fact what I was running into, forgot my tmpfs hack uses FAT thus the lower resolution.
Branch with my changes is at <lp:~gz/bzr/2.1.0rc1-set-mtime>
First rev changes the pyrex code to use SetFileTime, second addresses the two second thing. I'm not mad about how I've messed around with the tests, if you can think of a better way, don't pull that rev.
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin [gz] wrote:
>> How do you deal with FAT's 2 second window then? Surely it will go
>> backwards sometimes (to the start of the 2 seconds). Or is the time
>> supplied the time of one of the reverted files?
>
> This was in fact what I was running into, forgot my tmpfs hack uses FAT thus the lower resolution.
>
> Branch with my changes is at <lp:~gz/bzr/2.1.0rc1-set-mtime>
>
> First rev changes the pyrex code to use SetFileTime, second addresses the two second thing. I'm not mad about how I've messed around with the tests, if you can think of a better way, don't pull that rev.
Merged and updated the test cases. I used:
self.assertTrue
for the assertion. I also used your refactoring for running 2 tests, but
your 'if fset_mtime is not ...' was not correct.
I don't know if you noticed, but the function is not self-mutating (we
don't set osutils.fset_mtime, we set the global variable.) The
'fset_mtime' that we import is a local variable inside the fset_mtime
function, *not* the osutils.fset_mtime object.
That may be why you were confused earlier about why we have the if
clause. I'm sure there would be a way to do what you thought we were
doing. Like maybe:
def fset_mtime(...):
global fset_mtime
try:
if sys.platform == 'win32':
...
except ImportError:
fset_mtime = _utime_set_mtime
# Recurse now that we have set the *real* fset_mtime
return fset_mtime(...)
I'm not 100% comfortable with a function that replaces itself in the
outer scope. Which is why I used a global var instead.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
r9oAnA9xayAabVX
=WWvW
-----END PGP SIGNATURE-----
Martin Packman (gz) wrote : | # |
> That may be why you were confused earlier about why we have the if
> clause.
Yes, that's exactly it, I misread the function earlier. My problem with that file is every other function does laziness a different way... See osutils.
Found (hopefully) one last problem when doing some more testing. The new test in bt.test_transform fails without the compiled extension, despite the fact both the new TestFSetMtime tests pass. Fiddled around with the tests some more trying to work out where the problem was, pull from my branch again to see if you get the (marked expected) failure as well. The tests are back more like they were before, use `bzr diff -r4942.. bzrlib/tests` for a clean view of the changes.
I'm not sure how to fix it. Only idea so far is to change fset_mtime to set_mtime_
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin [gz] wrote:
>> That may be why you were confused earlier about why we have the if
>> clause.
>
> Yes, that's exactly it, I misread the function earlier. My problem with that file is every other function does laziness a different way... See osutils.
Since there is precedent, we could probably go the ..._thunk() route.
Note that _walkdirs_utf8 does the bulk of the actual work in the helper,
so the overhead is minimal, and we don't really need to replace the
top-level function.
>
> Found (hopefully) one last problem when doing some more testing. The new test in bt.test_transform fails without the compiled extension, despite the fact both the new TestFSetMtime tests pass. Fiddled around with the tests some more trying to work out where the problem was, pull from my branch again to see if you get the (marked expected) failure as well. The tests are back more like they were before, use `bzr diff -r4942.. bzrlib/tests` for a clean view of the changes.
I can reproduce the failure here by just deleting my extension. I ran
into something similar on Linux, which is why I added the .flush() line.
(It seems that linux was buffering output until the file.close() which
caused the mtime to get updated again.)
I'll note that the "utimens" has you pass the directory descriptor and
basename, rather than start from the full path. Which is an interesting
compromise. (You can hold the dir descriptor open for many files, but
don't have to have an open file to ensure the mtime gets set correctly.)
>
> I'm not sure how to fix it. Only idea so far is to change fset_mtime to set_mtime_
I think I've decided to go a different way, which is to just use
'os.utime()' for all code paths
In my timing tests, I can't find a benefit to custom extensions. And as
such, the reasonable default is to use the less complex implementation.
I've now pushed up that version. It certainly is a lot simpler overall.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
nYUAn2/
=necF
-----END PGP SIGNATURE-----
Martin Packman (gz) wrote : | # |
> Since there is precedent, we could probably go the ..._thunk() route.
Well, my point was more that there's all sorts of different precedent, which is why I get confused.
> I think I've decided to go a different way, which is to just use
> 'os.utime()' for all code paths
>
> In my timing tests, I can't find a benefit to custom extensions. And as
> such, the reasonable default is to use the less complex implementation.
>
> I've now pushed up that version. It certainly is a lot simpler overall.
Okay, this sounds like a good choice.
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin [gz] wrote:
> Review: Approve
>> Since there is precedent, we could probably go the ..._thunk() route.
>
> Well, my point was more that there's all sorts of different precedent, which is why I get confused.
>
>> I think I've decided to go a different way, which is to just use
>> 'os.utime()' for all code paths
>>
>> In my timing tests, I can't find a benefit to custom extensions. And as
>> such, the reasonable default is to use the less complex implementation.
>>
>> I've now pushed up that version. It certainly is a lot simpler overall.
>
> Okay, this sounds like a good choice.
So I feel like this has gotten a decent amount of review, even if Martin
<gz> hasn't given an official rating.
However, I was hoping to hear from Scott as to whether it actually helps.
Anyone care to give an official "approve" ?
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
Qz8AmwUI63rBa4+
=D/tr
-----END PGP SIGNATURE-----
Vincent Ladeuil (vila) wrote : | # |
Sounds good !
Just a nit:
8 +* Operations which update the working tree should now create all files
9 + with the same mtime.
When reading this I had slight doubt about whether modified files were also concerned.
I know they are, but saying create/modify instead of just create may make it clearer.
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Vincent Ladeuil wrote:
> Review: Approve
> Sounds good !
>
> Just a nit:
>
> 8 +* Operations which update the working tree should now create all files
> 9 + with the same mtime.
>
> When reading this I had slight doubt about whether modified files were also concerned.
>
> I know they are, but saying create/modify instead of just create may make it clearer.
Well, we create all files from scratch, and then rename them into place.
But sure, I'll reword it a bit.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
qLUAoMZXaBmeZm8
=AbBM
-----END PGP SIGNATURE-----
Scott James Remnant (Canonical) (canonical-scott) wrote : | # |
On Wed, 2010-01-06 at 10:54 +1100, Robert Collins wrote:
> On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
> >
> > Anyway, it doesn't seem like a huge overhead, as long as it does help
> > people.
>
> Scott reported this issue to me - Scott, can you try pulling this branch
> and seeing if it helps?
>
>
> https:/
>
Could you build a package with this applied and put in a PPA somewhere?
Scott
--
Scott James Remnant
<email address hidden>
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Scott James Remnant wrote:
> On Wed, 2010-01-06 at 10:54 +1100, Robert Collins wrote:
>
>> On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
>>> Anyway, it doesn't seem like a huge overhead, as long as it does help
>>> people.
>> Scott reported this issue to me - Scott, can you try pulling this branch
>> and seeing if it helps?
>>
>>
>> https:/
>>
> Could you build a package with this applied and put in a PPA somewhere?
>
> Scott
I think it should be in the bzr nightlies, and we'll be releasing
2.1.0rc1 within a week or so.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
6aoAnjIe6xQlM/
=oH05
-----END PGP SIGNATURE-----
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
John A Meinel wrote:
> Scott James Remnant wrote:
>> On Wed, 2010-01-06 at 10:54 +1100, Robert Collins wrote:
>
>>> On Tue, 2010-01-05 at 22:19 +0000, John A Meinel wrote:
>>>> Anyway, it doesn't seem like a huge overhead, as long as it does help
>>>> people.
>>> Scott reported this issue to me - Scott, can you try pulling this branch
>>> and seeing if it helps?
>>>
>>>
>>> https:/
>>>
>> Could you build a package with this applied and put in a PPA somewhere?
>
>> Scott
>
> I think it should be in the bzr nightlies, and we'll be releasing
> 2.1.0rc1 within a week or so.
>
> John
> =:->
>
Just to point you to it:
https:/
It seems to claim it has rev 4961, but that should include this (which
landed at rev 4940).
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkt
buQAoMF0I9T/
=+eGH
-----END PGP SIGNATURE-----
Preview Diff
1 | === modified file 'NEWS' |
2 | --- NEWS 2010-01-07 06:34:14 +0000 |
3 | +++ NEWS 2010-01-07 15:47:12 +0000 |
4 | @@ -71,6 +71,12 @@ |
5 | changed underneath it (like another autopack). Now concurrent |
6 | autopackers will properly succeed. (John Arbash Meinel, #495000) |
7 | |
8 | +* When operations update the working tree, all affected files should end |
9 | + up with the same mtime. (eg. when versioning a generated file, if you |
10 | + update the source and the generated file together, the generated file |
11 | + should appear up-to-date.) |
12 | + (John Arbash Meinel, Martin <gzlist>, #488724) |
13 | + |
14 | Improvements |
15 | ************ |
16 | |
17 | |
18 | === modified file 'bzrlib/tests/test_transform.py' |
19 | --- bzrlib/tests/test_transform.py 2009-12-04 06:13:25 +0000 |
20 | +++ bzrlib/tests/test_transform.py 2010-01-07 15:47:12 +0000 |
21 | @@ -1,4 +1,4 @@ |
22 | -# Copyright (C) 2006, 2007, 2008 Canonical Ltd |
23 | +# Copyright (C) 2006, 2007, 2008, 2009, 2010 Canonical Ltd |
24 | # |
25 | # This program is free software; you can redistribute it and/or modify |
26 | # it under the terms of the GNU General Public License as published by |
27 | @@ -17,6 +17,7 @@ |
28 | import os |
29 | from StringIO import StringIO |
30 | import sys |
31 | +import time |
32 | |
33 | from bzrlib import ( |
34 | bencode, |
35 | @@ -136,6 +137,29 @@ |
36 | transform.finalize() |
37 | transform.finalize() |
38 | |
39 | + def test_create_files_same_timestamp(self): |
40 | + transform, root = self.get_transform() |
41 | + self.wt.lock_tree_write() |
42 | + self.addCleanup(self.wt.unlock) |
43 | + # Roll back the clock, so that we know everything is being set to the |
44 | + # exact time |
45 | + transform._creation_mtime = creation_mtime = time.time() - 20.0 |
46 | + transform.create_file('content-one', |
47 | + transform.create_path('one', root)) |
48 | + time.sleep(1) # *ugly* |
49 | + transform.create_file('content-two', |
50 | + transform.create_path('two', root)) |
51 | + transform.apply() |
52 | + fo, st1 = self.wt.get_file_with_stat(None, path='one', filtered=False) |
53 | + fo.close() |
54 | + fo, st2 = self.wt.get_file_with_stat(None, path='two', filtered=False) |
55 | + fo.close() |
56 | + # We only guarantee 2s resolution |
57 | + self.assertTrue(abs(creation_mtime - st1.st_mtime) < 2.0, |
58 | + "%s != %s within 2 seconds" % (creation_mtime, st1.st_mtime)) |
59 | + # But if we have more than that, all files should get the same result |
60 | + self.assertEqual(st1.st_mtime, st2.st_mtime) |
61 | + |
62 | def test_hardlink(self): |
63 | self.requireFeature(HardlinkFeature) |
64 | transform, root = self.get_transform() |
65 | |
66 | === modified file 'bzrlib/transform.py' |
67 | --- bzrlib/transform.py 2009-12-03 05:57:41 +0000 |
68 | +++ bzrlib/transform.py 2010-01-07 15:47:12 +0000 |
69 | @@ -17,6 +17,7 @@ |
70 | import os |
71 | import errno |
72 | from stat import S_ISREG, S_IEXEC |
73 | +import time |
74 | |
75 | from bzrlib.lazy_import import lazy_import |
76 | lazy_import(globals(), """ |
77 | @@ -1023,6 +1024,7 @@ |
78 | self._limbo_children_names = {} |
79 | # List of transform ids that need to be renamed from limbo into place |
80 | self._needs_rename = set() |
81 | + self._creation_mtime = None |
82 | |
83 | def finalize(self): |
84 | """Release the working tree lock, if held, clean up limbo dir. |
85 | @@ -1133,6 +1135,7 @@ |
86 | f.writelines(contents) |
87 | finally: |
88 | f.close() |
89 | + self._set_mtime(name) |
90 | self._set_mode(trans_id, mode_id, S_ISREG) |
91 | |
92 | def _read_file_chunks(self, trans_id): |
93 | @@ -1145,6 +1148,15 @@ |
94 | def _read_symlink_target(self, trans_id): |
95 | return os.readlink(self._limbo_name(trans_id)) |
96 | |
97 | + def _set_mtime(self, path): |
98 | + """All files that are created get the same mtime. |
99 | + |
100 | + This time is set by the first object to be created. |
101 | + """ |
102 | + if self._creation_mtime is None: |
103 | + self._creation_mtime = time.time() |
104 | + os.utime(path, (self._creation_mtime, self._creation_mtime)) |
105 | + |
106 | def create_hardlink(self, path, trans_id): |
107 | """Schedule creation of a hard link""" |
108 | name = self._limbo_name(trans_id) |
This should be a basic fix for bug #488724.
It adds a new osutils function 'fset_mtime()' which takes a file descriptor (f.fileno()) and sets the mtime for that file.
It also updates TreeTransform so that for every file it creates it calls 'set_mtime()', and it sets it to 'time.time()' for the first file it creates.
I've manually validated on Windows that it does set mtime correctly.
I don't see any specific performance impact. Running 'bzr co --lightweight bzr.dev' about 9 times, I see a variation from 3.6s up to 12+s. However, I see the same variation both ways, and the minimum times are comparable. (The new code hit 3.6s, the old best was 3.7s.)
Times on an old Linux box are more reproducible with a small but measurable slowdown.
w/o:
8.16s user 1.97s system 93% cpu 10.802 total
8.18s user 1.81s system 95% cpu 10.496 total
8.13s user 1.99s system 82% cpu 12.231 total
w/:
8.07s user 2.13s system 89% cpu 11.346 total
8.20s user 2.09s system 91% cpu 11.302 total
8.25s user 2.06s system 86% cpu 11.907 total
The big notice is that sys time is about 5-10% bigger. Though it is a small portion of the overall time.
Anyway, it doesn't seem like a huge overhead, as long as it does help people.