Code review comment for lp:~jameinel/bzr/2.1.0rc1-set-mtime

Revision history for this message
John A Meinel (jameinel) wrote :

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.

« Back to merge proposal