1634437 - Call nsLookAndFeel::EnsureInit() before MSG_ConstructBrowser is received for new processes

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

In profiles of startup with fission (with Process preallocation, bug 1602757), we see that nsLookAndFeel::EnsureInit() takes a significant amount of time on Linux (and perhaps other platforms). Calling this earlier will significantly reduce the amount of time CreateBrowser takes to run, and so speed up the initial load into a new process (which happens a lot in Fission). On a high-end linux desktop, it takes ~37ms (including some font stuff that doesn't appear to be under it in the call tree): https://bit.ly/2xkYbID

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Doing this from an idle runnable dispatched during ContentChild startup might be a reasonable idea, as that way it won't block other important initialization, but might still get run before CreateBrowser in the preallocation case.

Comment 2

[Sean Feng [:sefeng]]

I am going to give this a try, by using Nika's suggestion.

Comment 3

[Sean Feng [:sefeng]]

I collect a new profile by loading cnn.com on my high-end laptop and this nsLookAndFeel::EnsureInit is much worse than 37ms on my laptop. Around 300 - 400ms for each web isolated process. Do we have to call it at each processe?

https://share.firefox.dev/322guzu

I wonder if we can just call it once in the main thread and pass it around. What do you think Nika?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I don't know the full scope of what nsLookAndFeel::EnsureInit does, but it would be lovely if we could call it only once in the parent process, and not need to ever initialize nsLookAndFeel within content processes. I unfortunately don't know how difficult that would be to do, as I believe this is configuring gtk data structures, which probably are difficult to send between processes.

This overhead might be eliminated by the work in bug 1411425, which should remove the need to ask gtk for theming information entirely. ni? heycam to check if this code is still run when widget.disable-native-theme-for-content is enabled, and if it is, why :-).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Even when native theming is off, we still need various other system bits for LookAndFeel. Those are for different things like system colors and so on. We might want to also make those non-native and such, but that seems like a bigger task than the scope of bug 1411425.

Comment 6

[Cameron McCormack (:heycam)]

The Gtk version of nsLookAndFeel::EnsureInit is definitely more heavyweight than the other themes. It creates a bunch of widgets just to look up various default styling information.

Sean, can you capture a profile with widget.disable-native-theme-for-content enabled? If there's still significant time being spent looking up system colors and so on, we could investigate whether it can be done lazily, but hopefully it's quick, and we can rely on bug 1411425 avoiding the bulk of the overhead.

Comment 7

[Sean Feng [:sefeng]]

Here's a profile with widget.disable-native-theme-for-content enabled: https://share.firefox.dev/2ZdOfvH

EnsureInit still took a significant amount for me.

I'd like to leave this bug open to try what Nika originally suggested as a short-term solution, do you want me to create a bug for investigating this done lazily? Sounds like in an ideal scenario, we want both lazy initializations and bug 1411425?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Initializing it lazily is not going to be enough I suspect, as we force initialization before styling starts so that we can access stuff off the main thread. So you're going to initialize it virtually everywhere.

Comment 9

[Sean Feng [:sefeng]]

Attachment: Bug 1634437 - Dispatch NativeInit as an idle runnable on Linux r=heycam

EnsureInit takes a long time to run on Linux, in addition to it's
timing, it also runs within a critical path of MSG_ConstructBrowser.
Scheduling it as an idle runnable gives it a chance to run
before MSG_ConstructBrowser.

Comment 10

[Sean Feng [:sefeng]]

If we try to do as a idle runnable, which is what the attached patch does, some of MSG_ConstructBrowser call dropped from ~350ms to ~7ms, if the runnable got a chance to run. https://share.firefox.dev/3flH3mT

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Sean Feng [:sefeng] from comment #7)

Here's a profile with widget.disable-native-theme-for-content enabled: https://share.firefox.dev/2ZdOfvH

Oh, right, we still create the Gtk nsLookAndFeel implementation even when not using nsNativeThemeGtk. (I was confusing theme and L&F.)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Initializing it lazily is not going to be enough I suspect, as we force initialization before styling starts so that we can access stuff off the main thread. So you're going to initialize it virtually everywhere.

Yes, fair point.

Re this being an idle runnable, I was going to ask whether since the SetXPCOMProcessAttributes message is async, we could just synchronously call NativeInit from ContentChild::InitXPCOM, but I guess there might be other messages that the parent wants to send to the newly created child that don't depend on the L&F have been initialized.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:sefeng, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 13

[Sean Feng [:sefeng]]

I haven't landed the patch yet because I don't know if it improves the performance. I tried to run some raptor page load tests and it didn't show any improvement.

I just kicked off some local browsertime runs to test the patch, so I should be able to have something to share soon and hopefully, it can reveal something useful.

Sorry about the lateness.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

How close are we to landing this?

Comment 15

[Sean Feng [:sefeng]]

So Last time I tried to run to browsertime on this and it didn't reveal any improvements. I checked the profiles and it turned out nsLookAndFeel::EnsureInit() method didn't always take a long time to run (at least on the testing machine that I used). And I haven't got a chance to figure out why.

I don't know if you have thoughts about this Randell. I could try again for sure.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

I see a clear 25-30ms improvement in the profiles with this patch

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33e2addbf47e
Dispatch NativeInit as an idle runnable on Linux r=heycam

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/33e2addbf47e