Optimize mozpack.copier.FileCopier.copy()

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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...
(Assignee)

Comment 2

4 years ago
Created attachment 8528767 [details]
MozReview Request: bz://1105128/gps
Attachment #8528767 - Flags: review?(mh+mozilla)
(Assignee)

Comment 3

4 years ago
/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
(Assignee)

Comment 4

4 years ago
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.
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 on attachment 8528767 [details]
MozReview Request: bz://1105128/gps

Skipping review of the scandir bits per comment 4
Attachment #8528767 - Flags: review?(mh+mozilla)
(Assignee)

Comment 8

4 years ago
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.
(Assignee)

Comment 10

4 years ago
https://reviewboard.mozilla.org/r/1035/#review775

I dropped the scandir patches before landing.
(Assignee)

Updated

4 years ago
Blocks: 1109409
(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.
(Assignee)

Comment 13

4 years ago
Comment on attachment 8528767 [details]
MozReview Request: bz://1105128/gps
Attachment #8528767 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8618727 [details]
MozReview Request: Bug 1105128 - Add scandir Python package
(Assignee)

Comment 15

4 years ago
Created attachment 8618728 [details]
MozReview Request: Bug 1105128 - Add scandir to virtualenv
(Assignee)

Comment 16

4 years ago
Created attachment 8618729 [details]
MozReview Request: Bug 1105128 - Incorporate scandir into install manifest processing
(Assignee)

Comment 17

4 years ago
Created attachment 8618730 [details]
MozReview Request: Bug 1105128 - Avoid excessive path normalization in FileCopier.copy()
(Assignee)

Comment 18

4 years ago
Created attachment 8618731 [details]
MozReview Request: Bug 1105128 - Alias os.path functions in local scope
(Assignee)

Comment 19

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

11 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.