Closed Bug 1602568 Opened 1 year ago Closed 1 year ago

Expand Rust PGO beyond linux64

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox73 fixed)

RESOLVED FIXED
mozilla73
Tracking Status
firefox73 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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)

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?

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?

Flags: needinfo?(dkeeler)
Blocks: 1543853
Depends on: 1602810

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.

Flags: needinfo?(dkeeler)

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! :-)

Assignee: nobody → dmajor

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.

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

Pushed by dmajor@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

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

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

Attachment #9114941 - Attachment is obsolete: true

== 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

Depends on: 1623085
No longer depends on: 1623085
You need to log in before you can comment on or make changes to this bug.