Bug 156178 - Web Inspector: Improve JavaScript pretty printing
Summary: Web Inspector: Improve JavaScript pretty printing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 117716 134003 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-04-04 13:29 PDT by Joseph Pecoraro
Modified: 2016-04-07 12:30 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Part 1 - EsprimaFormatter (177.25 KB, patch)
2016-04-04 14:03 PDT, Joseph Pecoraro
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Part 2 - Tests (393.08 KB, patch)
2016-04-04 14:04 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[IMAGE] Formatting Tool (201.06 KB, image/png)
2016-04-04 14:06 PDT, Joseph Pecoraro
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (880.22 KB, application/zip)
2016-04-04 14:29 PDT, Build Bot
no flags Details
[PATCH] Follow-up: WithStatement (4.44 KB, patch)
2016-04-06 22:55 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Follow-up: Correct formatted line endings from EsprimaFormatter (1.30 KB, patch)
2016-04-06 22:56 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Follow-up: Improve semicolon handling in for loop headers (10.80 KB, patch)
2016-04-06 22:57 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Follow-up: Pretty print JSON and fix up EsprimaFormatter error logic (3.87 KB, patch)
2016-04-06 22:58 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-04-04 13:29:23 PDT
* SUMMARY
Improve JavaScript pretty printing.

Goals:
- Performance: Faster on large inputs
- Correctness: Nested if/else/switch/try/for/while etc have always had problems
- Asynchronous: Having the ability to run non-synchronously (in a Worker) would prevent possible UI hangs
- Test Coverage: Our test coverage is currently pretty small for pretty-printing, we should expand it
Comment 1 Radar WebKit Bug Importer 2016-04-04 13:29:40 PDT
<rdar://problem/25535719>
Comment 2 Joseph Pecoraro 2016-04-04 14:03:37 PDT
Created attachment 275569 [details]
[PATCH] Part 1 - EsprimaFormatter
Comment 3 Joseph Pecoraro 2016-04-04 14:04:02 PDT
Created attachment 275570 [details]
[PATCH] Part 2 - Tests
Comment 4 Joseph Pecoraro 2016-04-04 14:06:01 PDT
Created attachment 275571 [details]
[IMAGE] Formatting Tool
Comment 5 Joseph Pecoraro 2016-04-04 14:16:08 PDT
In measuring the performance. Comparing the old formatter to the new formatter on jquery.min.js in the LayoutTests on my machine:

    Old:
    First run: 227ms
    Second run: 217ms
    Best run: 186ms

    New:
    First run: 181ms
    Second run: 97ms
    Best run: 76ms

The slow first run of the new formatter was better than the best case of the old formatter. And very soon after it improves dramatically. This is likely due to JavaScriptCore's really great performance with Esprima and not necessarily with how the formatter is written. Worst case in this scenario we get about a 20% improvement, best case we see up to a 2.5x improvement.

Both of these are using the same content builder, which along with building the output string builds the data we need to produce a SourceMap mapping the formatted content back to the original content.
Comment 6 BJ Burg 2016-04-04 14:20:01 PDT
Comment on attachment 275569 [details]
[PATCH] Part 1 - EsprimaFormatter

View in context: https://bugs.webkit.org/attachment.cgi?id=275569&action=review

> Source/WebInspectorUI/UserInterface/Controllers/FormatterSourceMap.js:39
> +    static fromSourceMapData({originalLineEndings, formattedLineEndings, mapping})

That is slick. Can we do more of this?

> Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:42
> +        })();

This seems to be handling both null return value and exceptions. Does esprima do both?
Comment 7 Build Bot 2016-04-04 14:29:55 PDT
Comment on attachment 275569 [details]
[PATCH] Part 1 - EsprimaFormatter

Attachment 275569 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1099620

New failing tests:
inspector/codemirror/prettyprinting-javascript.html
inspector/codemirror/prettyprinting-css-rules.html
inspector/codemirror/prettyprinting-css.html
Comment 8 Build Bot 2016-04-04 14:29:58 PDT
Created attachment 275576 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Joseph Pecoraro 2016-04-04 14:37:09 PDT
(In reply to comment #6)
> Comment on attachment 275569 [details]
> [PATCH] Part 1 - EsprimaFormatter
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275569&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/FormatterSourceMap.js:39
> > +    static fromSourceMapData({originalLineEndings, formattedLineEndings, mapping})
> 
> That is slick. Can we do more of this?

Yes, I've been moving in this direction. All of the async Worker/Proxy code uses it.

> > Source/WebInspectorUI/UserInterface/Workers/Formatter/EsprimaFormatter.js:42
> > +        })();
> 
> This seems to be handling both null return value and exceptions. Does
> esprima do both?

Looking at Esprima, parse() either returns an AST where the first node is a Program node, or it throws an exception. It should not return null.
Comment 10 Joseph Pecoraro 2016-04-04 14:48:47 PDT
*** Bug 134003 has been marked as a duplicate of this bug. ***
Comment 11 Joseph Pecoraro 2016-04-04 14:49:00 PDT
*** Bug 117716 has been marked as a duplicate of this bug. ***
Comment 12 Joseph Pecoraro 2016-04-04 14:56:20 PDT
Comment on attachment 275570 [details]
[PATCH] Part 2 - Tests

View in context: https://bugs.webkit.org/attachment.cgi?id=275570&action=review

> LayoutTests/inspector/formatting/resources/javascript-tests/generators-expected.js:4
> +function* gen() {}
> +function* gen() {
> +    1
> +}

We may want to change the "*" placement to be "function *gen".
Comment 13 Joseph Pecoraro 2016-04-06 22:55:52 PDT
Created attachment 275864 [details]
[PATCH] Follow-up: WithStatement
Comment 14 Joseph Pecoraro 2016-04-06 22:56:56 PDT
Created attachment 275865 [details]
[PATCH] Follow-up: Correct formatted line endings from EsprimaFormatter
Comment 15 Joseph Pecoraro 2016-04-06 22:57:51 PDT
Created attachment 275866 [details]
[PATCH] Follow-up: Improve semicolon handling in for loop headers
Comment 16 Joseph Pecoraro 2016-04-06 22:58:42 PDT
Created attachment 275867 [details]
[PATCH] Follow-up: Pretty print JSON and fix up EsprimaFormatter error logic
Comment 17 Joseph Pecoraro 2016-04-06 23:00:22 PDT
Now that I've tested this a bit more with real world content I found a few minor issues and  addressed them in small follow-up patches. Ultimately I intend to land this all at once, so these are just small patches on top of the existing Part 1 and 2 that will be squashed when landing.
Comment 18 Timothy Hatcher 2016-04-07 05:00:01 PDT
Follow ups look good.
Comment 19 Joseph Pecoraro 2016-04-07 12:30:35 PDT
http://trac.webkit.org/changeset/199168