918802 - Pretty printer can block Firefox on large files

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Anthony Ricaud (:rik)]

Minified files are often very large because they are in fact several files concatenated. We shouldn't block Firefox when pretty printing those.

Comment 1

[Rob Campbell [:rc] (:robcee)]

yup. hangs are bad.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We can possibly move parsing + generating to a worker, but if it is orion that is slow here, there isn't much we can do.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(Until Anton replaces it with Code Mirror, that is)

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-918802-pp-slow.patch

This about halves the speed.

The real problem is going from source map generator (what is returned by escodegen) to source map consumer (what we need to query info on the source map) because it is serializing the mappings into the base 64 VLQs and then parsing them right after that. This patch makes it so we only do this once, instead of twice.

The real fix is to add |SourceMapConsumer.fromSourceMapGenerator| which doesn't do all of this unnecessary serializing and parsing. I'm working on it, but it is a little trickier than I'd hoped and requires a bit of refactoring the internals of the source-map lib.

In the meantime, I think we can land this patch, and [leave-open] this bug.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 808051 [details] [diff] [review]
bug-918802-pp-slow.patch

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

::: toolkit/devtools/server/actors/script.js
@@ +2447,5 @@
>     * from _generatePrettyCodeAndMap goes the opposite way we want it to for
>     * debugging.
>     */
>    _invertSourceMap: function SA__invertSourceMap({ code, map }) {
> +    // XXX bug 918802: Monkey punch the source map consumer because parsing,

Punch the monkey!

I think you need a comma before "because".

@@ +2455,2 @@
>  
> +    map.setSourceContent(this._url, code);

Since we are modifying the call parameter here, it would be nice to mention this in the method comment, in case we ever try to use it under circumstances where this would pose a problem.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-918802-pp-slow.patch

Probably easier to review the source map changes at https://github.com/mozilla/source-map/pull/78 since they are in 3 nice little commits there.

This makes it so that the only substantial overhead is escodegen generating the source map, so we aren't doing any unnecessary parsing/generating of mappings.

Next step would be to move escodegen off main thread, but this is slightly complicated by the fact that it relies on our common js loader.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: bug-918802-pp-slow.patch

Changes in this patch from the last is that some .forEach loops in the source map library are turned into normal for loops for a speed up of about 250ms on the script I am testing on.

Starting to get marginal returns, the proper way forward is to put escodegen + source map lib in a worker. However, it is proving to be more difficult to do than I had hoped. Will keep looking into it.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Still probably easiest to review on github.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 809387 [details] [diff] [review]
bug-918802-pp-slow.patch

https://hg.mozilla.org/integration/fx-team/rev/c49d5912c0de

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Backed out because I landed an old version with logging.

https://hg.mozilla.org/integration/fx-team/rev/65b280441848

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Re-landed https://hg.mozilla.org/integration/fx-team/rev/4e9ccedf993c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e9ccedf993c

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip worker

Moves escodegen into a worker, more stuff to move in the worker with it, to get minimum jank.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip worker

This patch adds DevToolsUtils.yieldingEach so that we can iterate over large arrays (38k items in my test array) without causing jankiness.

XPCshell tests are failing because they don't like importing scripts in workers :-/

Comment 15

[Hubert B Manilla]

Hi, 
i also noticed that pretty print strips out all the comments in unminified files when it attempts to pretty print them.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Hubert B Manilla from comment #15)
> Hi, 
> i also noticed that pretty print strips out all the comments in unminified
> files when it attempts to pretty print them.

Yeah this is because Reflect.jsm doesn't have an option to include comment nodes in its AST. Filed bug 921163.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: wip worker

Updating UPGRADING file, small fixes here and there.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: worker

Woo! It works! Had to convert the xpcshell tests to mochitests because xpcshell barfs when you use ChromeWorkers. Also, couldn't dynamically eval the pretty printing + source mapping test's code, so I had to generate it by hand (added the scratchpad code required to do that as a comment in that code).

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

PR for source-map lib changes in this patch: https://github.com/mozilla/source-map/pull/79

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 811453 [details] [diff] [review]
worker

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

The improved responsiveness is fabulous! We desperately need a progress or activity indicator though, do we have a followup for that?

::: toolkit/devtools/DevToolsUtils.js
@@ +111,5 @@
> +  (function loop() {
> +    const start = Date.now();
> +
> +    while (i < len) {
> +      if (Date.now() - start > 16) {

This is good, but please add a comment about why 16 ms is used here.

::: toolkit/devtools/escodegen/UPGRADING.md
@@ +48,5 @@
> +   Then we need to build the browser version of escodegen:
> +
> +       $ cd /path/to/escodegen
> +       $ npm run-script build
> +       $ cat escodegen.browser.js >> /path/to/mozilla-central/toolkit/devtools/escodegen/escodegen.worker.js

This is getting quite long, we should look into creating a script to automate it as much as possible.

::: toolkit/devtools/escodegen/escodegen.worker.js
@@ +2,5 @@
> +/* -*- Mode: js; js-indent-level: 2; -*- */
> +/*
> + * Copyright 2011 Mozilla Foundation and contributors
> + * Licensed under the New BSD license. See LICENSE or:
> + * http://opensource.org/licenses/BSD-3-Clause

I'm sure it's OK, but have you checked with gerv or legal@ about the escodegen license? It may need to be added to about:license if it's not already there.

::: toolkit/devtools/server/actors/pretty-print-worker.js
@@ +1,2 @@
> +/* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2; js-indent-level: 2; -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

I don't think we are using this boilerplate any more since people can use their global editor configuration, but if you insist...

@@ +47,5 @@
> +      mappings: prettified.map._mappings
> +    });
> +  } catch (e) {
> +    self.postMessage({
> +      error: e.message + "\n" + e.stack

Can't error objects be properly serialized with the structured cloning algorithm?

::: toolkit/devtools/server/actors/script.js
@@ +474,5 @@
> +      this._prettyPrintWorker = new ChromeWorker(
> +        "resource://gre/modules/devtools/server/actors/pretty-print-worker.js");
> +
> +      this._prettyPrintWorker.addEventListener("error", error => {
> +        reportError(new Error(error));

Is this listener removed somewhere? I couldn't find it.

Also, is the reason for wrapping the error in a new object that structured cloning wouldn't work?

@@ +478,5 @@
> +        reportError(new Error(error));
> +      }, false);
> +
> +      if (wantLogging) {
> +        this._prettyPrintWorker.addEventListener("message", ({ data }) => {

I couldn't find where this listener was removed either.

@@ +3835,5 @@
>     */
>    _fetchSourceMap: function TS__fetchSourceMap(aAbsSourceMapURL, aScriptURL) {
>      return fetch(aAbsSourceMapURL, { loadFromCache: false })
>        .then(({ content }) => {
> +        dump("FITZGEN: content = '" + content + "'\n");

Oops :-)

::: toolkit/devtools/server/tests/unit/test_pretty_print-01.js
@@ -1,1 @@
> -/* -*- Mode: javascript; js-indent-level: 2; -*- */

You need to remove this and the other 2 tests from xpcshell.ini too, or tests will fail.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #20)
> Comment on attachment 811453 [details] [diff] [review]
> worker
> 
> Review of attachment 811453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The improved responsiveness is fabulous! We desperately need a progress or
> activity indicator though, do we have a followup for that?

Bug 921630

> 
> ::: toolkit/devtools/DevToolsUtils.js
> @@ +111,5 @@
> > +  (function loop() {
> > +    const start = Date.now();
> > +
> > +    while (i < len) {
> > +      if (Date.now() - start > 16) {
> 
> This is good, but please add a comment about why 16 ms is used here.

Will do. Chose 16 as that's the amount that you need to maintain 60fps, however this doesn't take into account the time to re-paint, and can't really. I think it is Close Enough(tm).

> 
> ::: toolkit/devtools/escodegen/UPGRADING.md
> @@ +48,5 @@
> > +   Then we need to build the browser version of escodegen:
> > +
> > +       $ cd /path/to/escodegen
> > +       $ npm run-script build
> > +       $ cat escodegen.browser.js >> /path/to/mozilla-central/toolkit/devtools/escodegen/escodegen.worker.js
> 
> This is getting quite long, we should look into creating a script to
> automate it as much as possible.

Filed bug 922324

> 
> ::: toolkit/devtools/escodegen/escodegen.worker.js
> @@ +2,5 @@
> > +/* -*- Mode: js; js-indent-level: 2; -*- */
> > +/*
> > + * Copyright 2011 Mozilla Foundation and contributors
> > + * Licensed under the New BSD license. See LICENSE or:
> > + * http://opensource.org/licenses/BSD-3-Clause
> 
> I'm sure it's OK, but have you checked with gerv or legal@ about the
> escodegen license? It may need to be added to about:license if it's not
> already there.

Bug 911995.

> @@ +47,5 @@
> > +      mappings: prettified.map._mappings
> > +    });
> > +  } catch (e) {
> > +    self.postMessage({
> > +      error: e.message + "\n" + e.stack
> 
> Can't error objects be properly serialized with the structured cloning
> algorithm?

Unfortunately, errors cannot be cloned.

> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +474,5 @@
> > +      this._prettyPrintWorker = new ChromeWorker(
> > +        "resource://gre/modules/devtools/server/actors/pretty-print-worker.js");
> > +
> > +      this._prettyPrintWorker.addEventListener("error", error => {
> > +        reportError(new Error(error));
> 
> Is this listener removed somewhere? I couldn't find it.

Will do; after talking with bent, it seems best to always do this if the worker is in chrome code.

> 
> Also, is the reason for wrapping the error in a new object that structured
> cloning wouldn't work?

Yup.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/d95ef98e7b05

Comment 23

[Richard Newman [:rnewman]]

Backed out for b-c failures.

https://hg.mozilla.org/integration/fx-team/rev/f2157693b1da

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/bdb2a66da7b5

Got tripped up in the mochitest changes. Relanded.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bdb2a66da7b5