Use multiple threads for install manifest / mozpack copier on Windows

RESOLVED FIXED in Firefox 47

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
I have a patch that makes install manifest processing 1.75x faster on Windows by using multiple threads.
(Assignee)

Comment 1

2 years ago
Created attachment 8720655 [details]
MozReview Request: Bug 1249210 - Install files using multiple threads on Windows; r?glandium

As previous measurements have shown, creating/appending files
on Windows/NTFS is slow because the CloseHandle() Win32 API takes
1-3ms to complete. This is apparently due to a fundamental issue
with NTFS extents. A way to work around this slowness is to use
multiple threads for I/O so file closing doesn't block execution
as much.

This commit updates the file copier to use a thread pool of 4
threads when processing file copies. Additional threads appear
to have diminishing returns.

On my i7-6700K, this reduces the time for processing the tests install
manifest (24,572 files) on Windows from ~22.0s to ~12.5s in the best
case.

Using the thread pool globally resulted in a performance regression
on Linux. Given the performance sensitivity of manifest copying,
I thought it best to implement a slightly redundant non-Windows
branch to preserve performance. For the record, that same machine
running Linux is capable of processing nearly the same install
manifest (24,616 files) in ~2.2s in the best case.

Review commit: https://reviewboard.mozilla.org/r/35413/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35413/
Attachment #8720655 - Flags: review?(mh+mozilla)
(Assignee)

Comment 2

2 years ago
Using a sample size of 1 for each run, time to process the _tests install manifest on Windows 8 Opt builders went from ~75s to ~39s with this patch.
(In reply to Gregory Szorc [:gps] from comment #1) 
> This commit updates the file copier to use a thread pool of 4
> threads when processing file copies. Additional threads appear
> to have diminishing returns.

Was this measured on your machine? I'm sure there's no perfect number here that produces best results everywhere, but I wonder if the sweet spot is the same on RelEng hardware.
(Assignee)

Comment 4

2 years ago
Both my machine and a sample size of 1 in automation show a 1.75-1.92x speedup. That's obviously better than 1.00. In the absence of evidence that shows this slows things down, let's land it and try to find the optimal value later?

The only thing I can think of that's wrong with this patch is we may want to add a low water mark for when threads kick in. It doesn't make sense to start a thread pool if installing 2 files.
(Assignee)

Comment 5

2 years ago
Comment on attachment 8720655 [details]
MozReview Request: Bug 1249210 - Install files using multiple threads on Windows; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35413/diff/1-2/
Attachment #8720655 - Flags: review?(mh+mozilla)
Comment on attachment 8720655 [details]
MozReview Request: Bug 1249210 - Install files using multiple threads on Windows; r?glandium

https://reviewboard.mozilla.org/r/35413/#review32373

::: python/mozbuild/mozpack/copier.py:387
(Diff revision 2)
> +        if os.name in ('nt', 'ce') and len(self) > 100:

sys.platform == 'win32'?

::: python/mozbuild/mozpack/copier.py:395
(Diff revision 2)
> +                    copy_results.append((destfile, f.result()))

f.result() could raise a TimeoutError. I won't claim to know how the futures API actually works, but reading the documentation, it seems it would just be better to do this loop after the ThreadPoolExecutor shutdown.

You could also do copy_results = [(destfile, f.result()) for destfile, f in fs]
(Assignee)

Comment 7

2 years ago
Comment on attachment 8720655 [details]
MozReview Request: Bug 1249210 - Install files using multiple threads on Windows; r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35413/diff/2-3/
Attachment #8720655 - Flags: review?(mh+mozilla)
Comment on attachment 8720655 [details]
MozReview Request: Bug 1249210 - Install files using multiple threads on Windows; r?glandium

https://reviewboard.mozilla.org/r/35413/#review32713
Attachment #8720655 - Flags: review?(mh+mozilla) → review+

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/58e1603e113d

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/58e1603e113d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
Blocks: 1252294
You need to log in before you can comment on or make changes to this bug.