1163840 - Generating stack in AsyncShutdown.jsm takes > 30ms on startup on Nexus 4

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Profiling Fennec startup on a Nexus 4, the function "Barrier/this.client.addBlocker" is called 9 times, with an average run of ~5.2ms. However, if I remove the block at [1], the average drops down to ~1.6ms. We should lazily initialize the stack to save >30ms on startup.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/asyncshutdown/AsyncShutdown.jsm?rev=f24c203c43f3#612

Comment 1

[David Teller [:Yoric] - still alive but not very active]

What do you mean by lazy initializing the stack? Using Cu.stack?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

On second thought, we should probably disable AsyncShutdown entirely on Fennec. Since Fennec has no shutdown sequence, AsyncShutdown does nothing useful on that app.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Lazy-init blocker stack in AsyncShutdown to save startup time (v1)

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #1)
> What do you mean by lazy initializing the stack? Using Cu.stack?

Yeah. For example, this patch adds an 'ensureStack' method to the blocker object, which lazy-inits the filename/lineNumber/stack fields when called later.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #2)
> On second thought, we should probably disable AsyncShutdown entirely on
> Fennec. Since Fennec has no shutdown sequence, AsyncShutdown does nothing
> useful on that app.

What's the best way to disable AsyncShutdown for only Android? We do exit Fennec using the normal shutdown path for tests and "quit Fennec" addons. But it's true that in general, AsyncShutdown is not used. So I think we can disable AsyncShutdown if we make sure that tests are not affected by potential incomplete shutdowns (e.g. startup perf tests may be affected by previous incomplete shutdowns).

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8604389 [details] [diff] [review]
Lazy-init blocker stack in AsyncShutdown to save startup time (v1)

Review of attachment 8604389 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ +655,5 @@
> +
> +      let ensureStack = function() {
> +        // Determine the filename and line number of the caller.
> +        let leaf = this.stackLeaf, frame = leaf;
> +        delete this.stackLeaf;

Nit: deleting properties is generally not a good idea. Better null them.

@@ +688,5 @@
>          stack: stack,
>          filename: filename,
> +        lineNumber: lineNumber,
> +        stackLeaf: stackLeaf,
> +        ensureStack: ensureStack,

Not a big fan of the post-initialization function `ensureStack`. I'd rather make it a function `getOrigin()` that returns `{lineNumber, filename, stack}`, without side-effects and without a property `stackLeaf`.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Lazy-init blocker stack in AsyncShutdown to save startup time (v2)

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #4)
> Comment on attachment 8604389 [details] [diff] [review]
> Lazy-init blocker stack in AsyncShutdown to save startup time (v1)
> 
> Review of attachment 8604389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +688,5 @@
> >          stack: stack,
> >          filename: filename,
> > +        lineNumber: lineNumber,
> > +        stackLeaf: stackLeaf,
> > +        ensureStack: ensureStack,
> 
> Not a big fan of the post-initialization function `ensureStack`. I'd rather
> make it a function `getOrigin()` that returns `{lineNumber, filename,
> stack}`, without side-effects and without a property `stackLeaf`.

Okay, I added a getOrigin() function that fills the values if necessary and returns an object.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8608246 [details] [diff] [review]
Lazy-init blocker stack in AsyncShutdown to save startup time (v2)

Review of attachment 8608246 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ +652,5 @@
> +      if (filename == "?" || lineNumber == -1 || stack === undefined) {
> +        stackLeaf = Components.stack;
> +      }
> +
> +      let getOrigin = function() {

I think it would be a bit nicer to lift `getOrigin` to the toplevel and use a trivial closure here.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Lazy-init blocker stack in AsyncShutdown to save startup time (v3)

Changed to use global function.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8609583 [details] [diff] [review]
Lazy-init blocker stack in AsyncShutdown to save startup time (v3)

Review of attachment 8609583 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with a trivial nit below.

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ +281,5 @@
> + * @param {string} stack Pre-supplied stack or undefined if unknown.
> + *
> + * @return object
> + */
> +function getOrigin(topFrame, filename, lineNumber, stack) {

As these are optional arguments, I'd prefer `filename = null`, `lineNumber = null`, `stack = null`. You'll need to check `filename == null`, etc. below.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/582fcb5a63d3

Comment 10

[Nicolas B. Pierron [:nbp]]

Could this patch cause the following regression on B2G:
  http://arewefastyet.com/regressions/#/regression/1720658

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/582fcb5a63d3

Comment 12

[David Teller [:Yoric] - still alive but not very active]

npb, how are these tests executed? This patch should make startup a little bit faster, but should have otherwise no impact.

Comment 13

[Nicolas B. Pierron [:nbp]]

These tests are the phone which are hanging on my desks.

Basically, they build B2G, by doing a repo --sync with mozilla-inbound instead of mozilla-central.
The benchmarks are then tested on the devices[1] with a marionnette tests case[2].

If you need, I can apply extra patches to the harness.

[1] https://github.com/nbp/arewefastyet/blob/master/driver/b2g-benchmark.sh
[2] https://github.com/nbp/gaia-ui-tests/blob/bench/gaiatest/tests/browser/benchmarks/test_bench_sunspider.py

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Does the benchmark measure startup and shutdown? It appears to me the benchmark should only be measuring individual tests, and I don't see why a startup/shutdown change would affect results from individual tests.