Rewrite makecab in Rust

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: ted, Assigned: ted)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
I wound up doing this while looking at bug 1323479. It turns out that dump_syms is only part of the slowness of buildsymbols on Windows, the other part is running `makecab` to compress the PDB files, and makecab is *really* slow. I did a simple Rust implementation that only implements what we need and it's significantly faster:
https://github.com/luser/rust-minidump

It speeds up the entire build by anywhere from 2-5 minutes depending on the build type:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=autoland&originalRevision=6f7b660eb31b&newProject=try&newRevision=7cc495f79da0&framework=2&filter=windows&showOnlyImportant=0

If you look at the subtest breakdown you can see that buildsymbols got faster, but it also shows wins for installer+package (because those run in parallel). I don't know that the numbers there are completely accurate because of that, but it's interesting.
(Assignee)

Comment 1

11 months ago
Oops, right link: https://github.com/luser/rust-makecab
Comment hidden (mozreview-request)
(Assignee)

Comment 3

11 months ago
Just for reference, I downloaded a build + symbols from my try build and verified that I was able to debug it locally. Debuggers are the only thing that consume the compressed pdb files currently.

Comment 4

11 months ago
mozreview-review
Comment on attachment 8824543 [details]
bug 1329320 - replace makecab with rust-makecab in symbolstore.

https://reviewboard.mozilla.org/r/103004/#review103920

This is more of a rubber stamp review since I didn't look at your Rust implementation in much detail since I don't know Rust... yet.

But, I trust you. And if this breaks things, it should be pretty obvious. So I think the barrier to change is low.
Attachment #8824543 - Flags: review?(gps) → review+

Comment 5

11 months ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dedf4f49b375
replace makecab with rust-makecab in symbolstore. r=gps

Comment 6

11 months ago
Backed out in https://hg.mozilla.org/integration/autoland/rev/a472aa6efe5ea5541d9d1dbc7d41db8ea0a76bcd for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=67339699&repo=autoland
Flags: needinfo?(ted)
(Assignee)

Comment 7

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31b6b05ba5b713a6fcac67dbde2eccd591a85dd
bug 1329320 - replace makecab with rust-makecab in symbolstore. r=gps
(Assignee)

Comment 8

11 months ago
I just needed to add a mock to one of the unit tests that was testing the Win32 implementation, since MAKECAB wasn't set.
Flags: needinfo?(ted)

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e31b6b05ba5b
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

11 months ago
Depends on: 1330250

Comment 10

11 months ago
Created attachment 8825741 [details] [diff] [review]
Followup to use TOOLTOOL_DIR for makecab

Since makecab is now fetched via tooltool, this should be using the TOOLTOOL_DIR when available to avoid problems for c-c and spidermonkey builds, similar to what was done for rust itself in bug 1324120.
Attachment #8825741 - Flags: review?(gps)

Updated

11 months ago
Assignee: ted → aleth

Updated

11 months ago
Assignee: aleth → ted

Updated

11 months ago
Attachment #8825741 - Flags: review?(gps) → review+

Comment 11

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f6833b434ba6d397a1819cdee42a6a065df35f
Bug 1329320 - Followup to use TOOLTOOL_DIR for makecab. r=gps

Comment 12

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81f6833b434b
(Assignee)

Comment 13

11 months ago
There's a nice visible drop in the buildsymbols time on the perfherder graphs.
buildbot win32: https://mzl.la/2jeQCu7
buildbot win64: https://mzl.la/2jM1BMj
taskcluster win32: https://mzl.la/2is5bsT
taskcluster win64: https://mzl.la/2invtJG
(Assignee)

Updated

10 months ago
Depends on: 1332147
You need to log in before you can comment on or make changes to this bug.