Closed Bug 835548 Opened 11 years ago Closed 11 years ago

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


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

Not set



blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed


(Reporter: cjones, Assigned: fabrice)



(Keywords: perf, Whiteboard: [FFOS_perf])


(1 file, 2 obsolete files)

For example in this profile

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?
Let's try that...
Assignee: nobody → fabrice
Attached patch patch v1 (obsolete) — Splinter Review
Not sure if that gives us any gain, but worth testing.
Attachment #707327 - Flags: feedback?(jones.chris.g)
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...
Attached patch patch v2 (obsolete) — Splinter Review
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?
Component: DOM: Apps → DOM: Mozilla Extensions
blocking-b2g: tef? → tef+
Attached patch patch v3Splinter Review
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 = { };
>+  }
>+ = 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
Backed out for mochitest-4 failures caused by something in this push.

206 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/webapps/test_bug_779982.html | Test timed out.
Keywords: perf
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.