1832022 - Consider to increase import-instr-limit

Status: ASSIGNED

(Firefox Build System :: General, enhancement, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently we have limit 10
https://searchfox.org/mozilla-central/rev/4e6970cd336f1b642c0be6c9b697b4db5f7b6aeb/build/moz.configure/lto-pgo.configure#308,310,312
Chrome for example uses higher 30
https://source.chromium.org/chromium/chromium/src/+/main:build/config/compiler/BUILD.gn;l=702;drc=a5158494b59e9053e3b56d777e2e0d48452aaaf3

10 -> 30
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=cec787d61490bd68de80145e2693baead8ac7d3f&newProject=try&newRevision=89055fe6d6edee0b9d0af2d24f11c891c00d8278&page=1&framework=13

Only important changes on Windows:
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=89055fe6d6edee0b9d0af2d24f11c891c00d8278&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=cec787d61490bd68de80145e2693baead8ac7d3f&page=1&showOnlyImportant=1

Installer size doesn't seem to increase massively
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&newProject=try&newRevision=89055fe6d6edee0b9d0af2d24f11c891c00d8278&framework=2&originalRevision=cec787d61490bd68de80145e2693baead8ac7d3f&page=3#tableLink-header-2637653140

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

glandium, you might have an opinion on this. Can you see any obvious possible issues with this?

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Serge might have an opinion too

Comment 3

[[:sergesanspaille]]

The default in LLVM is 100. I would be interested in a performance comparison to the default value. This flag only affects the number of imported function and limits the choice of the compiler, theoretically it should be used to bound compilation time and not affect performance in a bad manner. The control of binary size is a side effect of imperfect inliner.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Triggered tryserver builds also with 100:
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=cec787d61490bd68de80145e2693baead8ac7d3f&newProject=try&newRevision=b4409257aedcaaa7382b76c8abb34dfe8775d204

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

At least on Windows import-instr-limit=100 looks rather promising. Nothing super major, but still significant improvement
https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=b4409257aedcaaa7382b76c8abb34dfe8775d204&originalSignature=4586009&newSignature=4586009&framework=13&application=firefox&originalRevision=cec787d61490bd68de80145e2693baead8ac7d3f&page=1&showOnlyConfident=1

I didn't immediately see anything too worrisome in the binary size, on desktop at least.

Comment 6

[Timothy Nikkel (:tnikkel)]

I checked that import-instr-limit=100 doesn't regress bug 1824565 or bug 1827428 in try. 👍

Comment 7

[[:sergesanspaille]]

Perfherder comparison of switching back to the default for -import-instr-limit: https://treeherder.mozilla.org/perfherder/compare?originalProject=try&newRevision=217e391aa0ecea993ee7d787f05e22e6b80e4401&newProject=try&originalRevision=dedd2ec909e428a3891dea3c0796ffb35f9276d5&page=3&framework=2

Comment 8

[[:sergesanspaille]]

Attachment: Bug 1832022 - Reset to default -import-instr-limit threshold r=glandium,tnikkel

The compiler may be imperfect but it evolves, and hard-coded threshold
don't. Plus according to prefherder, the impact on size is within
reasonable bounds.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26904179b913
Reset to default -import-instr-limit threshold r=glandium

Comment 10

[Iulian Moraru]

Backed out for causing multiple crashes related to nsCOMPtr.

• Push with failures

• Failure log example 1

• Failure log example 2

• Backout link

This seems to affect the shippable platforms.

LATER EDIT: we found that this is the cause for some other failures:

• Failure log for crash signature [@ mozilla::dom::AutoJSAPI::AutoJSAPI]
• Failure log for btime failures
• Failure log for crash signature [@ encoding_rs::simd_funcs::load16_aligned]
• Failure log for crash signature [@ encoding_rs::mem::convert_str_to_utf16]
• Failure log for xpcshell failures on test_ext_scripting_contentScripts_css.js

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1832022 - Consider to increase import-instr-limit to 30, r=sergesanspaille

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f77e9c7c1ae0
Consider to increase import-instr-limit to 30, r=sergesanspaille

Comment 13

[Cristina Horotan [:chorotan]]

Backed out changeset f77e9c7c1ae0 (Bug 1832022) for causing mochitest failures

Backout: https://hg.mozilla.org/integration/autoland/rev/f8e518c6ab37a0144872f6158300e1de85034002

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f77e9c7c1ae00a5f13976e988f018edac974840d

Failure log: https://treeherder.mozilla.org/logviewer?job_id=427223350&repo=autoland&lineNumber=2564