1541518 - Fenix startup spends 226ms on "getScreenDepth"

Status: RESOLVED INVALID

(Core :: Performance, defect, P1)

Description

[Stuart Philp :sphilp]

According to Nimbledroid's icicle chart on the cold startup test, Fenix (when compared to Geckoview Example) is spending 226ms on GeckoAppShell.getScreenDepth, which seems to be

https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#1052

Seems like a long time given the definition above

Comment 1

[Stuart Philp :sphilp]

There's also sometimes quite a bit of time spent on getSystemColors (700-800ms...): https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#1236

Depending on the run you are looking at in Nimbledroid, ex. https://nimbledroid.com/my_apps/org.mozilla.fenix?p=7e40c4c8-eabc-4f9a-8bc8-c1b2c7d7dc8a#Icicle%20Graph

Comment 2

[Will Hawkins]

Thank you, sphilp, for filing this. I've seen this before, too. I will take ownership of this unless there are objections?

Comment 3

[Vicky Chin [:vchin]]

It's under our component Core:Performance so yes, free to take. Thank you!

Comment 4

[Chris Peterson [:cpeterson]]

Adding [geckoview:fenix:m5] whiteboard tag to track this issue for Fenix MVP.

Comment 5

[Will Hawkins]

We have this little gem in the documentation: https://developer.android.com/reference/android/view/Display.html#getPixelFormat()

Comment 6

[Chris Peterson [:cpeterson]]

Will, is this bug assigned to you because you're going to fix it or just because you are still profiling startup problems? Do I need to find a GeckoView Android engineer to fix this?

Comment 7

[Will Hawkins]

Interesting that you wrote this when you did. I was just about to provide an update:

This does not appear to be the source of any performance problem for the startup of Fenix. In this particular case, the function is called from the constructor of the gfxAndroidPlatform which is called from gfxPlatform::Init() the first time that the platform is required. Again, in this particular case, that happens when the main "browser" starts as a GeckoThread. The UI does not block waiting for the completion of the initialization of this Thread and, therefore, there are no noticeable affects.

In order to prove to myself that this was the case, I made the getScreenDepth() function return a constant value. It's runtime no longer showed up in the nimbledroid (ND) profile, but (non-intuitively) the startup time increased. Note: This is an artifact of the test platform. If you're interested in my current hypothesis about why this happens, I'll be happy to tell you offline.

One final note. This comes with the caveat that all of this is information provided with the best of my knowledge.

There is a way to optimize this function, if we want. We can easily remove the call to the Android SDK entirely as long as we are comfortable targeting devices that are platform level > 17. In that revision of the platform SDK, "This method is no longer supported. The result is always PixelFormat#RGBA_8888." As a result, we can simply set sScreenDepth to 32 and be happy.

I would enjoy the opportunity to talk to GV engineer about this to make sure that my understanding and description are accurate. In fact, that would be a good idea. Is there someone that I should reach out to?

I hope that this investigation helps!
Will

Comment 8

[Chris Peterson [:cpeterson]]

We can easily remove the call to the Android SDK entirely as long as we are comfortable targeting devices that are platform level > 17.

Good news! GeckoView requires API Level >= 18 so we can make that change for GeckoView (but not Fennec, because it requires API level >= 16).

we can simply set sScreenDepth to 32 and be happy.

If sScreenDepth is hard-coded to 32, then we might be able to remove GeckoAppShell.getScreenDepth() and simplify many of the callers of GeckoAppShell.getScreenDepth() and AndroidBridge::GetScreenDepth() by propagating the constant 32 into C++. Some callers have special cases to handle 16 or 24 bit screen depths.

I would enjoy the opportunity to talk to GV engineer about this to make sure that my understanding and description are accurate. In fact, that would be a good idea. Is there someone that I should reach out to?

Pinging @snorp or @rbarker in the #gv Slack channel would probably be a good place to start. If you would prefer a face-to-face meeting, just let me know and I can invite you to our next GV engineering team meeting.

Comment 9

[Will Hawkins]

I was going to reply to the comment above inline, but my mobile browser does not seem to like that. Overall, I agree with your suggestion but will write more first thing tomorrow. Thanks for the feedback!

Will

Comment 10

[Chris Peterson [:cpeterson]]

Adding [geckoview:fenix:m7] whiteboard tag because this bug is important but not strictly a Fenix MVP release blocker.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Will Hawkins from comment #7)

I would enjoy the opportunity to talk to GV engineer about this to make sure that my understanding and description are accurate. In fact, that would be a good idea. Is there someone that I should reach out to?

Your understanding seems accurate in that activity on the Gecko thread should generally not stop the UI. However, long startup operations on the Gecko thread do block us from rendering the first page, so we might still care about these problems.

Comment 12

[Will Hawkins]

It turns out that I cannot reproduce this on a Pixel 2 or Motorola g5 device. Even without the optimization in place, the function takes less than (at most) 5ms.

I am attempting to reproduce this on the Nexus 5 with older Android platform -- the setup that mimics ND.

Comment 13

[Will Hawkins]

We confirmed with ND that their profiler mistimes this function -- it does not take this amount of time. There are ways to optimize this function (as described above) but there is no reason to do those optimizations at this time.

Closing.