Closed Bug 1500154 Opened 2 years ago Closed 2 years ago
Significant amount of time spent in PLT stubs for geckoview on Android
When profiling various websites using focus+geckoview, I see a lot of time spent in plt stubs. They appear to mostly be from calling into libxul.so. After adding "-Wl,-Bsymbolic" to the linker flags for libxul.so, I see almost all of the plt stubs go away and around a +8% improvement in page load time in my local testing on the G5 and S7 (but we should do this more rigorously). There are other methods like symbol visibility or dynamic lists that could also do the same thing.
Mike is this a good idea?
Assignee: nobody → dpalmeiro
Priority: -- → P1
I thought we were using -Bsymbolic already, but it turns out we only do for ASAN. I'd support adding -Bsymbolic-functions on all builds using bfd ld, gold or lld (essentially, any build that supports it, like we do for e.g. -z text/relro/noexecstack/etc.).
This speeds up calls within libxul by avoiding PLT jumps.
Assignee: dpalmeiro → mbrubeck
Status: NEW → ASSIGNED
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/fda814029025 Link with -Bsymbolic-functions whenever possible. r=glandium
Denis, can you verify that this gave the expected performance increase on mozilla-central? I'll request uplift to beta if this is a significant perf win.
Sure, I can run those tests again. I just need to update my focus branch to the latest API changes first.
(In reply to Denis Palmeiro [:denispal] from comment #9) > Sure, I can run those tests again. I just need to update my focus branch to > the latest API changes first. Denis, in addition to the tests you ran before, it would be interesting to compare the before/after results for the Speedometer v2 benchmark on whatever device you're testing: https://mozilla.github.io/arewefastyet-speedometer/2.0/
Here are some local results on my S7 using the klarmArmDebug build variant of focus + revisions 20a550cfc08f and fda814029025 for GV. I did a cold load of each url about 50-100 times to try and smooth out the data since the noise was a bit high, but the new changes were consistently better even amidst the noise. I think these numbers are also a bit more in line with what I was expecting, and the 8% before was a little too optimistic. After all, the plt stubs were only consuming about 4-5% of the total runtime. URL 20a550cfc08f fda814029025 Difference ---------------------------------------------------------------------------- https://www.wsj.com 14050.305 13657.878 +2.79% https://www.news.com.au 10893.082 10508.636 +3.53% https://www.indiatimes.com 14579.412 14394.760 +1.27% https://www.usnews.com 4871.690 4808.370 +1.30% https://www.reddit.com 2298.370 2105.358 +8.40% https://www.nbcnews.com 16351.154 16048.509 +1.85% https://www.nytimes.com 16403.067 16892.385 -2.98% https://www.indianexpress.com 7180.510 7116.212 +0.90% https://www.cnn.com 15002.874 13232.591 +11.80% https://www.cnbc.com 13802.175 13439.730 +2.63% I also ran speedometer from browserbench.org comparing the two revisions, and I saw about +3% improvement. The score went from 10.1 to 10.4.
Comment on attachment 9019780 [details] Bug 1500154 - Link with -Bsymbolic-functions whenever possible. r=glandium [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: This patch measurably improves overall performance on some of the most popular Android devices. 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: Medium Why is the change risky/not risky? (and alternatives if risky): This patch changes linker options on all platforms. Linking with "-Bsymbolic-functions" can cause bugs in some rare cases (dynamically linked programs that compare function addresses across library boundaries). However, these should not occur in Gecko/Firefox, and this patch has been on nightly for a couple of weeks with no problems. A slightly lower-risk alternative would be to create a version of this patch that sets the new linker flag on Android/ARM only. Or we could decide not to backport the patch at all, and let it ride the trains. String changes made/needed: n/a
Attachment #9019780 - Flags: approval-mozilla-beta?
This is a low level change, I don't know that any regressions would be obvious, and we're halfway through beta 64. Is there major harm to letting this ride with 65?
(In reply to Julien Cristau [:jcristau] from comment #13) > This is a low level change, I don't know that any regressions would be > obvious, and we're halfway through beta 64. Is there major harm to letting > this ride with 65? That's fine. I will set 64=wontfix.
Comment on attachment 9019780 [details] Bug 1500154 - Link with -Bsymbolic-functions whenever possible. r=glandium OK, thanks Chris.
Attachment #9019780 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.