1500154 - Significant amount of time spent in PLT stubs for geckoview on Android

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Denis Palmeiro [:denispal]]

Attachment: Screenshot from 2018-10-12 11-51-13.png

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.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Mike is this a good idea?

Comment 2

[Mike Hommey [:glandium]]

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.).

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: Bug 1500154 - Link with -Bsymbolic-functions whenever possible. r=glandium

This speeds up calls within libxul by avoiding PLT jumps.

Comment 4

[Matt Brubeck (:mbrubeck)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb5206d4ec47e4d5d9d45621937fef828c88e8cb

https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=fb5206d4ec47e4d5d9d45621937fef828c88e8cb

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fda814029025
Link with -Bsymbolic-functions whenever possible. r=glandium

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fda814029025

Comment 7

[Matt Brubeck (:mbrubeck)]

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.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

\o/

Comment 9

[Denis Palmeiro [:denispal]]

Sure, I can run those tests again.  I just need to update my focus branch to the latest API changes first.

Comment 10

[Chris Peterson [:cpeterson]]

(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/

Comment 11

[Denis Palmeiro [:denispal]]

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 12

[Matt Brubeck (:mbrubeck)]

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

Comment 13

[Julien Cristau [:jcristau]]

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?

Comment 14

[Chris Peterson [:cpeterson]]

(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 15

[Julien Cristau [:jcristau]]

Comment on attachment 9019780 [details]
Bug 1500154 - Link with -Bsymbolic-functions whenever possible. r=glandium

OK, thanks Chris.

Comment 16

[Chris Peterson [:cpeterson]]

status-geckoview64=wontfix