[MERGE] Faster LocalTransport.put() and append() methods.

[Robert Collins]

On Fri, 2006-06-09 at 14:27 -0500, John Arbash Meinel wrote:
> Robert Collins wrote:
> > Seeking a +1.
> > 
> > This patch changes AtomicFile to be more targeted at the way we use it,
> > rather than a general safe-for-everything tool. It also changes
> > LocalTransport.append() to have less syscalls while being no longer.
> > 
> > The combination takes making a kernel_like_tree down from 7.3 seconds to
> > 1.5, which makes some benchmarks much more pleasant to run.
> > 
> > This is in my add() branch, where i'm currently fiddling stuff.
> > 
> > Rob
> > 
> 
> Where is AtomicFiles being used? By 'build_tree'? (Which I'm guessing is
> going through Transport.put() or somesuch)

Transport.put(), bzr ignore, and one other place I dont recall offhand.

> > 
> > === modified file 'bzrlib/atomicfile.py'
> > --- bzrlib/atomicfile.py	2005-12-18 04:08:46 +0000
> > +++ bzrlib/atomicfile.py	2006-06-09 10:38:59 +0000
> > @@ -15,10 +15,17 @@
> >  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> >  
> >  
> > -
> > +import codecs
> > +import errno
> > +import os
> > +import socket
> > +import sys
> >  from warnings import warn
> > +
> >  from bzrlib.osutils import rename
> > -import errno
> > +
> > +# not forksafe - but we dont fork.
> > +_pid = os.getpid()
> >  
> 
> ^- PEP8 needs extra blank lines. Also 'not forksafe' seems like
> something we need a FIXME. While 'bzr' doesn't fork, I don't think we
> can be that absolute for 'bzrlib'.

heh, yes it does. I'll update that to a FIXME once we resolve the modal
stuff.

> new_mode may be None, and under certain circumstances, we actually want
> to set the mode outside of the umask. The big reason for the chmod was
> so that on shared branches we would chmod the proper group bits, so that
> files would stay group readable, even if you had your umask at 0022
> (which is fairly common).
> Your changes break this.

yup. That wasn't intentional.

> Also, AtomicFile used to preserve the mode bits of the target file if
> you did not pass 'new_mode'. So in the working tree, if we were
> replacing a file with the execute bit set, the new file would also have
> the execute bit set.
> I don't think we use AtomicFile for the working directories, though I
> think we might think about it.

We dont use AtomicFile to alter files in the working directory, and have
not since the introduction of TreeTransforms - which have a similar
issue w.r.t. managing the mode of files.

> Anyway, I like the speed increase, but you seem to have done it by
> sacrificing the actual effect of the class.

Well, some of, not all the effect of the class ;).

> ...


> We should at least be 'chmod' ing if we create the file, see my earlier
> discussion about shared branches. And we need 'chmod' because of umask.
> Either that, or we always run with umask '0000', which I think is worse.

I dont see why we need to chmod *except* when making a branch new file
on disk.

We currently chmod in the following cases when a mode is specified...
AtomicFile
LocalTransport.append() with a new file
LocalTransport.append() with an existing file

Thats for .bzr files...

I think this is wrong, and we should only chmod when a mode is specified
AND:
the umask will result in a different mode on the output file if we
supply it ourselves
AND
we are making a new file not altering an existing one.

What do you think?


For files in the source tree Aaron and I have a patch which removes the
current chmod behaviour for a similar chmod-once approach (the aggregate
patch saved 30% on checkouts). The logic there should IMO be different,
because users expect umask to override things like checkouts. Or at
least, I do.

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: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060610/ff52554f/attachment.pgp