1624524 - 0.14 - 0.15% installer size (osx-shippable) regression on push 16f2eded7e001e3286715b31bb93fc8fc430d046 (Mon March 23 2020)

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=16f2eded7e001e3286715b31bb93fc8fc430d046

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

0.15% installer size osx-shippable opt instrumented 114,678,520.08 -> 114,852,105.75
0.14% installer size osx-shippable opt nightly 81,899,602.50 -> 82,017,907.92

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=25468

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Comment 1

[Lars T Hansen [:lth]]

We did replace a component written in C++ (maintained internally, not shared with anyone, not standards-compliant) with another written in Rust (maintained by the community, shared with lots of people, standards-compliant) and we want to make that stick.

But I'm frankly not sure why this code should be linked into the binary in a shippable build, though it seems both the old code and the new code are.

Benjamin & Christian - is there any reason the text->binary code is needed in a shipping build? It is only used from the shell function, which should not be available in a shippable build.

Nathan, could this be the consequence of some kind of tree-shaking link optimization that does not work on Rust code?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

A search on Searchfox shows that wasmTextToBinary is used in DOM/devtools testing, using the SpecialPowers ability:
https://searchfox.org/mozilla-central/search?q=wasmTextToBinary&case=false&regexp=false&path=

If these tests are run on automation (including for shippable builds), then it'll be hard to strip the function's code from the binary. Otherwise (these tests are not run for shipped builds), we could remove it from the list of available testing functions, conditionally on configure flags (if local/test build; there ought to be flags for this).

(Clearing Nathan's NI, since the function is explicitly exported, there's no more suspicion for a tree-shaking link optimization failure.)

Comment 3

[Christian Holler (:decoder)]

From a fuzzing perspective, there is no requirement to have this in release builds. Even if we used it (afaik we don't right now), we could hide it behind the --enable-fuzzing build flag.

Comment 4

[Ryan Hunt [:rhunt]]

I don't think this functionality is needed outside the JS shell, I'll work to conditionally compile it.

Comment 5

[Ryan Hunt [:rhunt]]

Just an update, I'm still working on this and have it mostly working. Hunting down a pesky Windows only failure now.

Comment 6

[Ryan Hunt [:rhunt]]

Attachment: Bug 1624524 - Only compile wasmTextToBinary and wasmCodeOffsets in JS Shell. r?bbouvier

Comment 7

[Ryan Hunt [:rhunt]]

Attachment: Bug 1624524 - Remove uses of wasmTextToBinary in non-JS shell tests. r?bbouvier

Depends on D68650

Comment 8

[Jens Stutte [:jstutte]]

Hi Lars, can you please (let) set a priority here?

Comment 9

[Lars T Hansen [:lth]]

Ryan, what news?

Comment 10

[Julien Cristau [:jcristau]]

Eric, can someone else review D68650 while Nathan's out?

Comment 11

[Eric Rahm [:erahm]]

(In reply to Julien Cristau [:jcristau] from comment #10)

Eric, can someone else review D68650 while Nathan's out?

Sure, redirected to glandium.

Comment 12

[Ryan Hunt [:rhunt]]

Should be ready to land once review is finished.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bab62d72587a
Only compile wasmTextToBinary and wasmCodeOffsets in JS Shell. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/c5f9f6b565e2
Remove uses of wasmTextToBinary in non-JS shell tests. r=bbouvier

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bab62d72587a
https://hg.mozilla.org/mozilla-central/rev/c5f9f6b565e2

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:rhunt, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Ryan Hunt [:rhunt]]

Comment on attachment 9136479 [details]
Bug 1624524 - Remove uses of wasmTextToBinary in non-JS shell tests. r?bbouvier

Beta/Release Uplift Approval Request

• User impact if declined: Slight installer size increase on OSX. Looks like ~100KB.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a build configuration only change to conditionally compile test-only code. There are no behavioral changes.
• String changes made/needed:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9136478 [details]
Bug 1624524 - Only compile wasmTextToBinary and wasmCodeOffsets in JS Shell. r?bbouvier

Reduces installer size by not building code only used by the JS shell. Approved for 76.0b5.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63a9a835c34e
https://hg.mozilla.org/releases/mozilla-beta/rev/f4c37f32c8a6