1602568 - Expand Rust PGO beyond linux64

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[(Away)]

After Rust PGO landed on linux64, it seems we didn't get around to enabling it elsewhere.

I did a try push on Windows, and it's currently busted:

[task 2019-12-09T17:45:16.035Z] 17:45:16     INFO -  z:/task_1575906750/fetches/clang/bin/lld-link.exe -NOLOGO -DLL -OUT:osclientcerts.dll -PDB:osclientcerts.pdb -SUBSYSTEM:WINDOWS,6.01 -MACHINE:X64 Unified_cpp_dynamic-library0.obj ./module.res  -LARGEADDRESSAWARE -RELEASE -DEBUG -OPT:REF,ICF -guard:cf,nolongjmp   z:\task_1575906750\build\src\obj-firefox\x86_64-pc-windows-msvc\release\osclientcerts_static.lib  -DEF:osclientcerts.dll.def  user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib secur32.lib crypt32.lib kernel32.lib ncrypt.lib userenv.lib ws2_32.lib
[task 2019-12-09T17:45:16.035Z] 17:45:16     INFO -  lld-link: error: undefined symbol: __llvm_profile_instrument_range
[task 2019-12-09T17:45:16.035Z] 17:45:16     INFO -  >>> referenced by /rustc/4560ea788cb760f0a34127156c78e2552949f734\src\libstd\sync\once.rs:225
[task 2019-12-09T17:45:16.035Z] 17:45:16     INFO -  >>>               osclientcerts_static.lib(osclientcerts_static-ae78308f5a205a7c.osclientcerts_static.c168d664-cgu.0.rcgu.o):

Is osclientcerts.dll special for some reason? How come we aren't linking against clang_rt.profile-x86_64.lib from the PGO gen_ldflags? (Other binaries in the log do have it)

Comment 1

[(Away)]

Interesting: osclientcerts says NO_PGO = True. Perhaps we should apply the same to its RustLibrary('osclientcerts-static') ? Does the build system even know how to selectively disable PGO for Rust?

Comment 2

[(Away)]

The builds succeed if I remove that NO_PGO. Seems like that would be the path of least resistance, if I can get away with it.

:keeler, do you happen to recall why osclientcerts/dynamic-library/moz.build has NO_PGO? I recall a long time ago there were concerns about PGO in security code, but I'm not sure if that's still a thing in 2019. On the other hand, if it was more of an "eh, it just didn't seem necessary", then would you mind if I removed that line?

Comment 3

[Dana Keeler (she/her) [:keeler]]

That was basically a copy/paste from https://searchfox.org/mozilla-central/source/security/manager/ssl/tests/unit/pkcs11testmodule/moz.build#20 which it looks like was added because we don't actually ship the test pkcs11 module. Since we do ship the osclientcerts module on Windows, I'd say we should enable PGO for it if that works.

Comment 4

[(Away)]

which it looks like was added because we don't actually ship the test pkcs11 module.

Hah, blame says that was me, I had completely forgotten! :-)

Comment 5

[(Away)]

Attachment: Enable Rust PGO on linux32 and Windows

Comment 6

[(Away)]

Attachment: Remove NO_PGO from osclientcerts library

If we turn on Rust PGO, the build system can't deal with the mixture of this NO_PGO library along with its PGO'ed Rust dependency. It seems that this NO_PGO might have been a copy/paste artifact anyway, so let's go ahead and remove it.

Comment 7

[(Away)]

Attachment: Remove unused option --enable-cross-pgo

Nothing looks at the value of this option. We seem to have settled on the --enable-profile-{generate,use}=cross approach instead.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/611a2b145da2
Remove NO_PGO from osclientcerts library r=keeler
https://hg.mozilla.org/integration/autoland/rev/65d69ba8d88a
Enable Rust PGO on linux32 and Windows r=firefox-build-system-reviewers,chmanchester

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/611a2b145da2
https://hg.mozilla.org/mozilla-central/rev/65d69ba8d88a

Comment 10

[Phabricator Automation]

Comment on attachment 9114941 [details]
Remove unused option --enable-cross-pgo

Revision D56588 was moved to bug 1603141. Setting attachment 9114941 [details] to obsolete.

Comment 11

[Marian Raiciof [:marauder]]

== Change summary for alert #24461 (as of Thu, 12 Dec 2019 08:36:59 GMT) ==

Improvements:

13% perf_reftest_singletons parent-basic-singleton.html windows10-64-shippable opt e10s stylo 94.12 -> 81.42
13% perf_reftest_singletons parent-basic-singleton.html windows10-64-shippable-qr opt e10s stylo 94.70 -> 81.97
13% perf_reftest_singletons tiny-traversal-singleton.html windows10-64-shippable-qr opt e10s stylo 926.29 -> 807.05
13% perf_reftest_singletons tiny-traversal-singleton.html windows10-64-shippable opt e10s stylo 912.72 -> 796.00
12% perf_reftest_singletons tiny-traversal-singleton.html windows7-32-shippable opt e10s stylo 886.88 -> 784.92
10% perf_reftest_singletons parent-basic-singleton.html windows7-32-shippable opt e10s stylo 92.66 -> 83.49
7% perf_reftest_singletons coalesce-2.html windows10-64-shippable opt e10s stylo 139.65 -> 130.54
6% perf_reftest_singletons coalesce-2.html windows10-64-shippable-qr opt e10s stylo 143.75 -> 134.56
6% perf_reftest_singletons coalesce-2.html windows7-32-shippable opt e10s stylo 133.57 -> 125.55
5% perf_reftest_singletons coalesce-1.html windows10-64-shippable-qr opt e10s stylo 178.70 -> 169.06
5% perf_reftest_singletons abspos-reflow-1.html windows10-64-shippable-qr opt e10s stylo 59.77 -> 56.71
5% perf_reftest_singletons coalesce-1.html windows7-32-shippable opt e10s stylo 165.08 -> 156.69

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24461