1109409 - Optimize mozpack.files.BaseFile.copy() on Windows

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #1105128 +++

As part of working on bug 1105128, I profiled install manifest performance from a *clean objdir* and noticed that a lot of time on Windows was spent in Win32 system calls called from shutil.copy2 called from mozpack.files.BaseFile.copy() in the branch where the destination path does not exist.

It's easy to see why: shutil.copyfile (https://hg.python.org/cpython/file/323f51ce8d86/Lib/shutil.py#l66) open 2 file handles then invokes copyfileobj (https://hg.python.org/cpython/file/323f51ce8d86/Lib/shutil.py#l46) which does 16k read + writes.

I suspect rewriting the code to use ctypes to call into CopyFile() will yield much faster performance.

This is a low-hanging fruit until we support symlinks on Windows.

Comment 1

[Nathan Froyd [:froydnj]]

It looks like this is hugely worthwhile, testing something like:

mkdir obj-with-shutil obj-with-copyfile
(cd obj-with-shutil; ../configure)
(cd obj-with-copyfile; ../configure)
(cd obj-with-shutil; ../mach build install-manifests)
(cd obj-with-copyfile; ../mach build install-manifests)

showed that the CopyFile-based version completes in ~1/2 the time, or about a full minute faster (Windows 7, x86-64 build, SSD).

I'll try to clean up the patches and post them later.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: improve mozpack.BaseFile.copy() performance on Windows

mopack.BaseFile.copy() performs a generic read/write file copy.  Windows
has an explicit CopyFile() call that tests have shown to be
significantly faster.  Let's use that instead via the magic of ctypes.

I guess this should really only matter for a small amount of builds, but it's
still >1% of build time on my machine, so it's probably worthwhile.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 8535663 [details] [diff] [review]
improve mozpack.BaseFile.copy() performance on Windows

Review of attachment 8535663 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with s/Error/TypeError/ and a Try run to ensure we're not triggering the mismatched types code path (which wouldn't be surprising).

::: python/mozbuild/mozpack/files.py
@@ +50,5 @@
> +            CopyFileW(src, dest, False)
> +        elif isinstance(src, str) and isinstance(dest, str):
> +            CopyFileA(src, dest, False)
> +        else:
> +            raise Error('mismatched path types!')

Where does "Error" come from? I suspect it is undefined.

You probably want TypeError. See https://docs.python.org/2/library/exceptions.html

@@ +153,5 @@
>  
>          if can_skip_content_check:
>              if getattr(self, 'path', None) and getattr(dest, 'path', None):
> +                _copyfile(self.path, dest.path)
> +                shutil.copystat(self.path, dest.path)

Do we need copystat() on Windows? I thought CopyFile copied attributes as well. This could be a follow-up if I'm right: I don't want to wait any longer to see this land!

Comment 4

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65e5fdd27316

(In reply to Gregory Szorc [:gps] from comment #3)
> @@ +153,5 @@
> >  
> >          if can_skip_content_check:
> >              if getattr(self, 'path', None) and getattr(dest, 'path', None):
> > +                _copyfile(self.path, dest.path)
> > +                shutil.copystat(self.path, dest.path)
> 
> Do we need copystat() on Windows? I thought CopyFile copied attributes as
> well. This could be a follow-up if I'm right: I don't want to wait any
> longer to see this land!

I'll check, but it looks like copystat copied things other than, say, what GetFileAttributes returns (mtime, for instance).  Fodder for a follow-up.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65e5fdd27316