Closed Bug 918802 Opened 7 years ago Closed 7 years ago

Pretty printer can block Firefox on large files

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: rik, Assigned: fitzgen)

References

()

Details

(Keywords: hang)

Attachments

(2 files, 5 obsolete files)

Minified files are often very large because they are in fact several files concatenated. We shouldn't block Firefox when pretty printing those.
yup. hangs are bad.
Keywords: hang
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.
Whiteboard: [needs-profiling]
(Until Anton replaces it with Code Mirror, that is)
Assignee: nobody → nfitzgerald
Attached patch bug-918802-pp-slow.patch (obsolete) — Splinter Review
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.
Attachment #808051 - Flags: review?(past)
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.
Attachment #808051 - Flags: review?(past) → review+
Attached patch bug-918802-pp-slow.patch (obsolete) — Splinter Review
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.
Attachment #808051 - Attachment is obsolete: true
Attachment #808693 - Flags: review?(dcamp)
Whiteboard: [needs-profiling] → [leave-open]
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.
Attachment #808693 - Attachment is obsolete: true
Attachment #808693 - Flags: review?(dcamp)
Attachment #809387 - Flags: review?(dcamp)
Still probably easiest to review on github.
Attachment #809387 - Flags: review?(dcamp) → review+
Backed out because I landed an old version with logging.

https://hg.mozilla.org/integration/fx-team/rev/65b280441848
Attached patch wip worker (obsolete) — Splinter Review
Moves escodegen into a worker, more stuff to move in the worker with it, to get minimum jank.
Attached patch wip worker (obsolete) — Splinter Review
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 :-/
Attachment #810206 - Attachment is obsolete: true
Hi, 
i also noticed that pretty print strips out all the comments in unminified files when it attempts to pretty print them.
(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.
Attached patch wip worker (obsolete) — Splinter Review
Updating UPGRADING file, small fixes here and there.
Attachment #810314 - Attachment is obsolete: true
Attached patch workerSplinter Review
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).
Attachment #810834 - Attachment is obsolete: true
Attachment #811453 - Flags: review?(past)
PR for source-map lib changes in this patch: https://github.com/mozilla/source-map/pull/79
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.
Attachment #811453 - Flags: review?(past) → review+
(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.
Backed out for b-c failures.

https://hg.mozilla.org/integration/fx-team/rev/f2157693b1da
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/bdb2a66da7b5

Got tripped up in the mochitest changes. Relanded.
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bdb2a66da7b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.