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

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: fabrice)

Tracking

({perf})

unspecified
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [FFOS_perf])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Can we .jsm'ize some of this code for win?
(Assignee)

Comment 2

5 years ago
Let's try that...
Assignee: nobody → fabrice
(Assignee)

Comment 3

5 years ago
Created attachment 707327 [details] [diff] [review]
patch v1

Not sure if that gives us any gain, but worth testing.
Attachment #707327 - Flags: feedback?(jones.chris.g)
(Assignee)

Updated

5 years ago
Whiteboard: [FFOS_perf]
Comment on attachment 707327 [details] [diff] [review]
patch v1

No difference.
Attachment #707327 - Flags: feedback?(jones.chris.g) → feedback-
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.
> 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.
Outside of my front yard, but there's no way to do more of that work statically?
(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...
(Assignee)

Comment 9

5 years ago
Created attachment 707435 [details] [diff] [review]
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.
Attachment #707327 - Attachment is obsolete: true
Attachment #707435 - Flags: feedback?(jones.chris.g)
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".
Attachment #707435 - Flags: feedback?(jones.chris.g) → feedback+
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?
blocking-b2g: --- → tef?

Updated

5 years ago
Component: DOM: Apps → DOM: Mozilla Extensions
blocking-b2g: tef? → tef+
(Assignee)

Comment 12

5 years ago
Created attachment 707901 [details] [diff] [review]
patch v3
Attachment #707435 - Attachment is obsolete: true
Attachment #707901 - Flags: review?(justin.lebar+bug)
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.  :)
Attachment #707901 - Flags: review?(justin.lebar+bug) → review+
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
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → affected
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0fc79d7a8b
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.
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8921a4c3c433
Keywords: perf
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4cc3add4dffc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/8921a4c3c433
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/3cbfd3725b47
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → mozilla21
Blocks: 835547
status-b2g18-v1.0.1: --- → fixed
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.