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

RESOLVED FIXED in Firefox 65

Status

defect
P1
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: denispal, Assigned: mbrubeck)

Tracking

(Blocks 1 bug)

Trunk
mozilla65
ARM
Android
Dependency tree / graph

Firefox Tracking Flags

(geckoview64 wontfix, firefox64 wontfix, firefox65 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

Reporter

Description

7 months ago
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
Flags: needinfo?(mh+mozilla)
Priority: -- → P1
Reporter

Updated

7 months ago
Whiteboard: [qf]

Updated

7 months ago
Whiteboard: [qf] → [qf: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.).
Flags: needinfo?(mh+mozilla)
Assignee

Comment 3

7 months ago
This speeds up calls within libxul by avoiding PLT jumps.
Assignee

Updated

7 months ago
Assignee: dpalmeiro → mbrubeck
Status: NEW → ASSIGNED

Comment 5

7 months ago
Pushed by mbrubeck@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fda814029025
Link with -Bsymbolic-functions whenever possible. r=glandium

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fda814029025
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee

Comment 7

7 months ago
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.
Flags: needinfo?(dpalmeiro)
See Also: → 1503330
Reporter

Comment 9

7 months ago
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/
Reporter

Comment 11

7 months ago
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.
Flags: needinfo?(dpalmeiro)
Reporter

Updated

7 months ago
Blocks: 1505256
Assignee

Comment 12

7 months ago
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?
See Also: 1503330
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-
status-geckoview64=wontfix

Updated

5 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 65 → mozilla65
You need to log in before you can comment on or make changes to this bug.