835548 - BrowserElementParent.js factory and related mozapps "stuff" taking ~100 samples (130ms) on critical startup path

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For example in this profile

http://people.mozilla.com/~bgirard/cleopatra/#report=d9d022b3178b5e8e72e2036cf040b316a5eca471

there's a big chunk of time under window_manager.js appendFrame().  This is the gaia code that creates the <iframe mozbrowser> and inserts it into the DOM.  It's really deep in the stack there so you have to be a bit patient ;).

There's some unavoidable overhead here since this is a pretty heavyweight operation, but I'm sure we can optimize away a lot of this.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Can we .jsm'ize some of this code for win?

Comment 2

[[:fabrice] Fabrice Desré]

Let's try that...

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: patch v1

Not sure if that gives us any gain, but worth testing.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 707327 [details] [diff] [review]
patch v1

No difference.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I notice that addMessageListener() is taking a nontrivial amount of time, about 10 samples.  definteMethod() and defineDOMRequestMethod() are contributing 7 samples.  So at least half the overhead in BEP factory init is just JS stuff that we should be able to avoid.

We're burning another 25 samples here in getAppByManifestURL() and in cloneAsMozIApplication().  Maybe we can reduce the number of calls there, or optimize a bit further.

Comment 6

[Justin Lebar (not reading bugmail)]

> So at least half the overhead in BEP factory init is just JS stuff that we should be able 
> to avoid.

defineMethod and defineDOMRequestMethod() need to run on the <iframe mozbrowser>.  I'm not sure how you intend to do these ahead of time.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Outside of my front yard, but there's no way to do more of that work statically?

Comment 8

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> Outside of my front yard, but there's no way to do more of that work
> statically?

I don't see a simple way to short-circuit this work.  Perhaps if we had better insight into exactly what is slow, that might suggest a solution.  For example, perhaps calling XPCNativeWrapper.unwrap() is slow.  If so, we could call it once, instead of N times.

There's a Right Solution here, which is to define a new XPIDL interface for <iframe mozbrowser>.  Then when you create the iframe all the methods will be there.  But that's not happening anytime in the next week...

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Instead of having N message manager messages named browser-element-api:messageX, this patch uses a single message browser-element-api:call and moves messageX to be a parameter of the call. This let us do only one mm.addMessageListener() which are costly. I applied this technique both in the parent and the child.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 707435 [details] [diff] [review]
patch v2

This is removing on average about 40 samples from the critical startup path across b2g/Template process.  (One measurement was 10/30, and one was 20/20, which is a little odd.  But definitely significant.)

When I measure outside the profiler with a stopwatch, there's a noticeable improvement.  I think part of that is that this patch makes Template app early startup sometimes fast enough that it gets "lucky" and doesn't hit the async-sync wait interval described in bug 835681 comment 2.

I saw this in one trial with the profiler and there were a total of about 200 samples on Template startup.  In another trial, we didn't get lucky and were back up to about 290.  I suspect that with profiling disabled we're more consistently "lucky".

Comment 11

[Justin Lebar (not reading bugmail)]

Comment on attachment 707435 [details] [diff] [review]
patch v2

Can you please do an hg cp of BEP.js to BEP.jsm so we don't lose all our blame history?

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 707901 [details] [diff] [review]
patch v3

Looks good to me.

> Bug 835548 - BrowserElementParent.js factory and related mozapps "stuff" taking ~100 samples (130ms) on critical startup path

I don't know if you intended to change this commit message before you pushed, but please indicate what you're changing, not just what the problem is.

> function sendAsyncMsg(msg, data) {
>-  sendAsyncMessage('browser-element-api:' + msg, data);
>+  if (!data) {
>+    data = { };
>+  }
>+
>+  data.call = msg;

I'd prefer this wasn't named "call"; that sounds like a verb, and it's also
overloaded with "browser-element-api:call".  "msg_name" would work for me.

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.js

Please update the explanatory comment in BEP.js to explain what's going on
here.  The comment talks about "BrowserElementParent" which no longer exists in
this file!

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.jsm
>copy from dom/browser-element/BrowserElementParent.js
>copy to dom/browser-element/BrowserElementParent.jsm
>--- a/dom/browser-element/BrowserElementParent.js
>+++ b/dom/browser-element/BrowserElementParent.jsm

>+this.BrowserElementParentBuilder = {
>+  create: function create(frameLoader, hasRemoteFrame) {
>+    return new BrowserElementParent(frameLoader, hasRemoteFrame);
>+  }
> }

Please add a brief explanatory comment explaining what's going on and why
there's both BEP.js and BEP.jsm.

Have fun harmonizing this with Vivien's patch.  :)

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Comment 15

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0fc79d7a8b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-4 failures caused by something in this push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/220ee1b126c3

https://tbpl.mozilla.org/php/getParsedLog.php?id=19298749&tree=Mozilla-Inbound

206 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/webapps/test_bug_779982.html | Test timed out.

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8921a4c3c433

Comment 18

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4cc3add4dffc

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8921a4c3c433

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/3cbfd3725b47