1105128 - Optimize mozpack.copier.FileCopier.copy()

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

mozpack.copier.FileCopier.copy() is an operation performed on every build. We should make what it is doing (syncing the state of the filesystem from manifests) as fast as possible.

I wrote some patches to make it a little faster. Stay tuned...

Comment 1

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=ee1ac151aced

Comment 2

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1105128/gps

Comment 3

[Gregory Szorc [:gps]]

/r/1037 - Bug 1105128 - Avoid excessive path normalization in FileCopier.copy()
/r/1039 - Bug 1105128 - Alias os.path functions in local scope
/r/1041 - Bug 1105128 - Add scandir Python package
/r/1043 - Bug 1105128 - Add scandir to virtualenv
/r/1045 - Bug 1105128 - Incorporate scandir into install manifest processing

Pull down these commits:

hg pull review -r ee1ac151aced450f3fe91d88ab4449a461bcae88

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/1035/#review553

The scandir changes probably need some flushing out on Try and with the virtualenv integration. The first 2 patches should be ready for review.

I hope to fire up my Windows machine and get some performance numbers. I'm optimistic scandir makes install manifest processing significantly faster. If not, we should probably drop scandir since it offers no compelling advantage.

Comment 5

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1037/#review767

Ship It!

Comment 6

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/1039/#review769

Ship It!

::: python/mozbuild/mozpack/copier.py
(Diff revision 1)
> +        dirname = os.path.dirname

You might as well do from os.path import normpath, join, dirname. You can do it locally or globally, it probably doesn't matter much.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8528767 [details]
MozReview Request: bz://1105128/gps

Skipping review of the scandir bits per comment 4

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/1039/#review773

::: python/mozbuild/mozpack/copier.py
(Diff revision 1)
> +        dirname = os.path.dirname

I'd rather not. We import mozpath at the global level. I think choosing to alias one over the other globally is a bit odd.

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/def38e936db8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dfe0838798c

Comment 10

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/1035/#review775

I dropped the scandir patches before landing.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #8)
> https://reviewboard.mozilla.org/r/1039/#review773
> 
> ::: python/mozbuild/mozpack/copier.py
> (Diff revision 1)
> > +        dirname = os.path.dirname
> 
> I'd rather not. We import mozpath at the global level. I think choosing to
> alias one over the other globally is a bit odd.

You could still have used import in the function.

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8d579034e13e for Windows test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4488235&repo=mozilla-inbound

Comment 14

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1105128 - Add scandir Python package

Comment 15

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1105128 - Add scandir to virtualenv

Comment 16

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1105128 - Incorporate scandir into install manifest processing

Comment 17

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1105128 - Avoid excessive path normalization in FileCopier.copy()

Comment 18

[Gregory Szorc [:gps]]

Attachment: MozReview Request: Bug 1105128 - Alias os.path functions in local scope

Comment 19

[Gregory Szorc [:gps]]

The first commits landed and I have no intention of following up with scandir at this time. But someone should eventually. Closing this out because partially landed bugs months old bugs are confusing.