Page MenuHomePhabricator

Upgrade ResourceLoader JS minifier to support ES6
Closed, ResolvedPublic

Description

Upgrade ResourceLoader's JS Minifier to support ES6

Event Timeline

Krinkle triaged this task as Medium priority.Jan 25 2021, 7:16 PM
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Change 593934 had a related patch set uploaded (by Catrope; owner: Jforrester):
[mediawiki/core@master] [DNM] resourceloader: Replace JavaScriptMinifier with Minify

https://gerrit.wikimedia.org/r/593934

Change 411387 had a related patch set uploaded (by Catrope; owner: Jforrester):
[mediawiki/core@master] [DNM] Benchmark comparing JavaScriptMinifier and MinifyJS

https://gerrit.wikimedia.org/r/411387

Benchmark results:

1vagrant@vagrant:/vagrant/mediawiki$ php maintenance/benchmarks/compareJavaScriptMinifiers.php
2Comparing JavaScriptMinifier and MinifyJS over 10 iterations on 22 files.
3resources/lib/jquery/jquery.js (c0952713; 274 KiB)
4 JavaScriptMinifier — mean: 384.57 ms max: 488.82 ms rate: 2.60 /s output size: 138 KiB
5 MinifyJS — mean: 234.18 ms max: 264.27 ms rate: 4.27 /s output size: 138 KiB
6resources/lib/moment/moment.js (d38ff349; 170 KiB)
7 JavaScriptMinifier — mean: 211.07 ms max: 212.64 ms rate: 4.74 /s output size: 95 KiB
8 MinifyJS — mean: 61.64 ms max: 64.69 ms rate: 16.22 /s output size: 94 KiB
9resources/lib/ooui/oojs-ui-core.js (dad36a60; 415 KiB)
10 JavaScriptMinifier — mean: 361.65 ms max: 363.50 ms rate: 2.77 /s output size: 171 KiB
11 MinifyJS — mean: 2471.11 ms max: 2606.90 ms rate: 0.40 /s output size: 170 KiB
12resources/lib/ooui/oojs-ui-widgets.js (5b440e05; 175 KiB)
13 JavaScriptMinifier — mean: 176.22 ms max: 179.01 ms rate: 5.67 /s output size: 76 KiB
14 MinifyJS — mean: 571.74 ms max: 589.22 ms rate: 1.75 /s output size: 76 KiB
15resources/lib/ooui/oojs-ui-windows.js (d756645c; 115 KiB)
16 JavaScriptMinifier — mean: 103.59 ms max: 138.64 ms rate: 9.65 /s output size: 41 KiB
17 MinifyJS — mean: 195.90 ms max: 230.51 ms rate: 5.10 /s output size: 40 KiB
18resources/lib/pako/pako_deflate.js (1289ef06; 125 KiB)
19 JavaScriptMinifier — mean: 127.57 ms max: 130.69 ms rate: 7.84 /s output size: 43 KiB
20 MinifyJS — mean: 207.61 ms max: 226.70 ms rate: 4.82 /s output size: 43 KiB
21resources/lib/qunitjs/qunit.js (5c66acc7; 186 KiB)
22 JavaScriptMinifier — mean: 239.57 ms max: 244.34 ms rate: 4.17 /s output size: 100 KiB
23 MinifyJS — mean: 104.62 ms max: 106.47 ms rate: 9.56 /s output size: 100 KiB
24resources/lib/sinonjs/sinon.js (1636edf4; 210 KiB)
25 JavaScriptMinifier — mean: 272.84 ms max: 278.47 ms rate: 3.67 /s output size: 113 KiB
26 MinifyJS — mean: 42.81 ms max: 44.07 ms rate: 23.36 /s output size: 160 KiB
27resources/lib/vue/vue.common.dev.js (158c9e53; 313 KiB)
28 JavaScriptMinifier — mean: 440.87 ms max: 466.50 ms rate: 2.27 /s output size: 191 KiB
29 MinifyJS — mean: 1062.01 ms max: 1097.15 ms rate: 0.94 /s output size: 191 KiB
30resources/lib/vue/vue.common.prod.js (bc41beb5; 91 KiB)
31 JavaScriptMinifier — mean: 314.83 ms max: 330.20 ms rate: 3.18 /s output size: 91 KiB
32 MinifyJS — mean: 45.72 ms max: 63.95 ms rate: 21.87 /s output size: 91 KiB
33extensions/3D/modules/three/three.js (14c96de7; 1.01 MiB)
34 JavaScriptMinifier — mean: 1623.01 ms max: 1675.28 ms rate: 0.62 /s output size: 730 KiB
35 MinifyJS — mean: 474.18 ms max: 515.94 ms rate: 2.11 /s output size: 727 KiB
36extensions/CodeEditor/modules/ace/ace.js (95b0f780; 702 KiB)
37 JavaScriptMinifier — mean: 1154.19 ms max: 1205.26 ms rate: 0.87 /s output size: 482 KiB
38 MinifyJS — mean: 329.42 ms max: 344.20 ms rate: 3.04 /s output size: 480 KiB
39extensions/CodeEditor/modules/ace/mode-php.js (776d2e29; 581 KiB)
40 JavaScriptMinifier — mean: 292.32 ms max: 340.70 ms rate: 3.42 /s output size: 483 KiB
41 MinifyJS — mean: 549.18 ms max: 561.91 ms rate: 1.82 /s output size: 483 KiB
42extensions/CodeEditor/modules/ace/mode-php_laravel_blade.js (b24fc5f1; 588 KiB)
43 JavaScriptMinifier — mean: 311.58 ms max: 332.62 ms rate: 3.21 /s output size: 488 KiB
44 MinifyJS — mean: 563.84 ms max: 576.62 ms rate: 1.77 /s output size: 488 KiB
45extensions/CodeEditor/modules/ace/worker-xquery.js (6fff22ab; 3.34 MiB)
46 JavaScriptMinifier — mean: 5587.48 ms max: 5654.80 ms rate: 0.18 /s output size: 1.75 MiB
47 MinifyJS — mean: 65581.92 ms max: 66583.79 ms rate: 0.02 /s output size: 1.74 MiB
48extensions/Graph/lib/vega2/vega.js (92d0f434; 618 KiB)
49 JavaScriptMinifier — mean: 1150.84 ms max: 1189.06 ms rate: 0.87 /s output size: 421 KiB
50 MinifyJS — mean: 486.24 ms max: 495.74 ms rate: 2.06 /s output size: 419 KiB
51extensions/Kartographer/lib/external/mapbox/mapbox-lib.js (c2260fd8; 525 KiB)
52 JavaScriptMinifier — mean: 677.43 ms max: 707.80 ms rate: 1.48 /s output size: 262 KiB
53 MinifyJS — mean: 309.50 ms max: 311.94 ms rate: 3.23 /s output size: 260 KiB
54extensions/TimedMediaHandler/resources/mwembed/lib/binPlayers/ogv.js/ogv-decoder-video-av1.js (f22c4569; 912 KiB)
55 JavaScriptMinifier — mean: 4194.16 ms max: 4244.98 ms rate: 0.24 /s output size: 913 KiB
56 MinifyJS — mean: 249.50 ms max: 253.46 ms rate: 4.01 /s output size: 912 KiB
57extensions/TimedMediaHandler/resources/videojs/alt/video.core.min.js (9ab3ab78; 212 KiB)
58 JavaScriptMinifier — mean: 563.50 ms max: 576.02 ms rate: 1.77 /s output size: 212 KiB
59 MinifyJS — mean: 119.31 ms max: 121.11 ms rate: 8.38 /s output size: 212 KiB
60extensions/VisualEditor/lib/ve/dist/visualEditor-rebase.js (b1912746; 2.32 MiB)
61 JavaScriptMinifier — mean: 3013.94 ms max: 3046.76 ms rate: 0.33 /s output size: 1.19 MiB
62 MinifyJS — mean: 83.16 ms max: 89.02 ms rate: 12.02 /s output size: 2.32 MiB
63extensions/VisualEditor/lib/ve/dist/visualEditor.js (65ec015e; 2.19 MiB)
64 JavaScriptMinifier — mean: 2489.80 ms max: 2554.97 ms rate: 0.40 /s output size: 1.08 MiB
65 MinifyJS — mean: 112.99 ms max: 124.52 ms rate: 8.85 /s output size: 2.17 MiB
66extensions/Wikibase/client/data-bridge/dist/data-bridge.app.js (6615f253; 209 KiB)
67 JavaScriptMinifier — mean: 559.54 ms max: 562.91 ms rate: 1.79 /s output size: 207 KiB
68 MinifyJS — mean: 147.67 ms max: 162.85 ms rate: 6.77 /s output size: 209 KiB

For most inputs, the new minifier (MinifyJS) is faster than the old one (JavaScriptMinifier). For some inputs, it's much much slower (e.g. oojs-ui-core, 362ms vs 2471ms) while producing output of about the same size. For some inputs, it's dramatically faster while producing much larger output, typically close to the original input size. This happens when the input is already minified, and also in some cases where it's not, so maybe there's an "is this already minified" detection heuristic that's off.

I imagine upstream would take our patches if we could work out what was going wrong. :-)

For the OOUI input case, most of the slowness occurs on this line:

// multi-line comments
$this->registerPattern('/\n?\/\*(!|.*?@license|.*?@preserve).*?\*\/\n?/s', $callback);

Commenting it out speeds up minification of oojs-ui-core.js from 2372ms to 475ms.

This appears to be a regex that preserves license comments in the output. The regex looks for multiline comments that start with /*! or that contain @license or @preserve anywhere in the comment, and $callback is a function that replaces it with a placeholder. The next line replaces all multiline comments with nothing. I suspect (but haven't had the time to verify yet) that what the slow inputs have in common is that they contain many multiline comments, and that one of the non-greedy match everything (.*?) parts of this regex causes catastrophic backtracking.

I will investigate more tomorrow.

Since I'm here, I figured it'd be worth pointing out a few caveats about the minifier, most of which probably won't really be a surprise:

  • This grew from a few very tiny regular expression. It is a lot more complex now, but it's essentially still just a bunch of regexes. They're usually faster than any PHP userland code could parse JS syntax, but they're limited in their ability to process code (e.g. finding matching closing brackets in nested structures is something regexes are not great at...)
  • Since it's just a bunch of regexes, it does not parse or validate the original or produced code, so it may silently produce invalid results.
  • One of the most common ways in which it could silently fail is when the PCRE limits (pcre.backtrack_limit & pcre.recursion_limit - see http://php.net/manual/en/pcre.configuration.php) are configured too low. I've optimized the regexes a bunch and have not recently seen bug reports that indicate it still happens frequently (if at all), but it is technically still possible, given low enough limits & code written in a specific way to trigger large pcre recursion/backtracking.
  • A JS-based minifier should be much superior than this (or any other) PHP-based minifier. While I started to build it for exactly the same reasons that apply to MediaWiki (be able to run with minimal fuss & additional infrastructure), it might makes sense to attempt to shell out to a JS compiler for WMF sites (if at all possible), and use a PHP-based solution as fallback, when that infrastructure is not available (for many 3rd party installs)

(I had no idea this was going on - feel free to ping me if there's anything I can help with)

https://github.com/matthiasmullie/minify/commit/8538190f4ab21f77c938e51109547f0e943f7d44 would probably fix the slowness in that regex. Thanks for tracking that down!

Yes, that's pretty much exactly what I did (with minor modifications, will comment on github).

I'm also about to submit a PR that dramatically speeds up the minifier for large inputs by reducing the amount of string copying. In the initial benchmark, you can see that extensions/CodeEditor/modules/ace/worker-xquery.js takes 66 seconds. Profiling it showed that the majority of the time was spent in substr(). I think this is because it's a large file (3 MB) with lots of things (mostly strings) that are placeholderized. With my patch, that file takes 0.7 seconds.

Full benchmarks with my modifications (which I'll submit to github after I finish eating breakfast):

1Comparing JavaScriptMinifier and MinifyJS over 10 iterations on 22 files.
2resources/lib/jquery/jquery.js (c0952713; 274 KiB)
3 JavaScriptMinifier — mean: 280.71 ms max: 334.76 ms rate: 3.56 /s output size: 138 KiB
4 MinifyJS — mean: 61.89 ms max: 79.54 ms rate: 16.16 /s output size: 138 KiB
5resources/lib/moment/moment.js (d38ff349; 170 KiB)
6 JavaScriptMinifier — mean: 165.17 ms max: 176.24 ms rate: 6.05 /s output size: 95 KiB
7 MinifyJS — mean: 34.88 ms max: 36.98 ms rate: 28.67 /s output size: 94 KiB
8resources/lib/ooui/oojs-ui-core.js (dad36a60; 415 KiB)
9 JavaScriptMinifier — mean: 276.50 ms max: 284.16 ms rate: 3.62 /s output size: 171 KiB
10 MinifyJS — mean: 56.91 ms max: 60.38 ms rate: 17.57 /s output size: 170 KiB
11resources/lib/ooui/oojs-ui-widgets.js (5b440e05; 175 KiB)
12 JavaScriptMinifier — mean: 121.12 ms max: 133.54 ms rate: 8.26 /s output size: 76 KiB
13 MinifyJS — mean: 23.99 ms max: 26.63 ms rate: 41.68 /s output size: 76 KiB
14resources/lib/ooui/oojs-ui-windows.js (d756645c; 115 KiB)
15 JavaScriptMinifier — mean: 66.43 ms max: 69.14 ms rate: 15.05 /s output size: 41 KiB
16 MinifyJS — mean: 16.49 ms max: 23.89 ms rate: 60.66 /s output size: 40 KiB
17resources/lib/pako/pako_deflate.js (1289ef06; 125 KiB)
18 JavaScriptMinifier — mean: 93.81 ms max: 96.35 ms rate: 10.66 /s output size: 43 KiB
19 MinifyJS — mean: 22.98 ms max: 25.44 ms rate: 43.52 /s output size: 43 KiB
20resources/lib/qunitjs/qunit.js (5c66acc7; 186 KiB)
21 JavaScriptMinifier — mean: 174.10 ms max: 207.90 ms rate: 5.74 /s output size: 100 KiB
22 MinifyJS — mean: 35.81 ms max: 42.30 ms rate: 27.92 /s output size: 100 KiB
23resources/lib/sinonjs/sinon.js (1636edf4; 210 KiB)
24 JavaScriptMinifier — mean: 191.39 ms max: 226.77 ms rate: 5.22 /s output size: 113 KiB
25 MinifyJS — mean: 43.76 ms max: 56.06 ms rate: 22.85 /s output size: 117 KiB
26resources/lib/vue/vue.common.dev.js (158c9e53; 313 KiB)
27 JavaScriptMinifier — mean: 405.14 ms max: 521.12 ms rate: 2.47 /s output size: 191 KiB
28 MinifyJS — mean: 66.50 ms max: 77.82 ms rate: 15.04 /s output size: 191 KiB
29resources/lib/vue/vue.common.prod.js (bc41beb5; 91 KiB)
30 JavaScriptMinifier — mean: 233.81 ms max: 280.44 ms rate: 4.28 /s output size: 91 KiB
31 MinifyJS — mean: 23.55 ms max: 25.54 ms rate: 42.46 /s output size: 91 KiB
32extensions/3D/modules/three/three.js (14c96de7; 1.01 MiB)
33 JavaScriptMinifier — mean: 1064.77 ms max: 1113.52 ms rate: 0.94 /s output size: 730 KiB
34 MinifyJS — mean: 143.00 ms max: 147.00 ms rate: 6.99 /s output size: 727 KiB
35extensions/CodeEditor/modules/ace/ace.js (95b0f780; 702 KiB)
36 JavaScriptMinifier — mean: 765.58 ms max: 846.01 ms rate: 1.31 /s output size: 482 KiB
37 MinifyJS — mean: 120.49 ms max: 129.06 ms rate: 8.30 /s output size: 480 KiB
38extensions/CodeEditor/modules/ace/mode-php.js (776d2e29; 581 KiB)
39 JavaScriptMinifier — mean: 198.15 ms max: 211.90 ms rate: 5.05 /s output size: 483 KiB
40 MinifyJS — mean: 148.14 ms max: 157.84 ms rate: 6.75 /s output size: 483 KiB
41extensions/CodeEditor/modules/ace/mode-php_laravel_blade.js (b24fc5f1; 588 KiB)
42 JavaScriptMinifier — mean: 203.06 ms max: 210.52 ms rate: 4.92 /s output size: 488 KiB
43 MinifyJS — mean: 148.94 ms max: 158.31 ms rate: 6.71 /s output size: 488 KiB
44extensions/CodeEditor/modules/ace/worker-xquery.js (6fff22ab; 3.34 MiB)
45 JavaScriptMinifier — mean: 3913.34 ms max: 4000.26 ms rate: 0.26 /s output size: 1.75 MiB
46 MinifyJS — mean: 744.04 ms max: 842.13 ms rate: 1.34 /s output size: 1.74 MiB
47extensions/Graph/lib/vega2/vega.js (92d0f434; 618 KiB)
48 JavaScriptMinifier — mean: 816.18 ms max: 973.31 ms rate: 1.23 /s output size: 421 KiB
49 MinifyJS — mean: 143.54 ms max: 151.74 ms rate: 6.97 /s output size: 419 KiB
50extensions/Kartographer/lib/external/mapbox/mapbox-lib.js (c2260fd8; 525 KiB)
51 JavaScriptMinifier — mean: 454.51 ms max: 484.24 ms rate: 2.20 /s output size: 262 KiB
52 MinifyJS — mean: 101.54 ms max: 106.89 ms rate: 9.85 /s output size: 260 KiB
53extensions/TimedMediaHandler/resources/mwembed/lib/binPlayers/ogv.js/ogv-decoder-video-av1.js (f22c4569; 912 KiB)
54 JavaScriptMinifier — mean: 2947.47 ms max: 3054.50 ms rate: 0.34 /s output size: 913 KiB
55 MinifyJS — mean: 128.97 ms max: 134.67 ms rate: 7.75 /s output size: 912 KiB
56extensions/TimedMediaHandler/resources/videojs/alt/video.core.min.js (9ab3ab78; 212 KiB)
57 JavaScriptMinifier — mean: 369.80 ms max: 381.42 ms rate: 2.70 /s output size: 212 KiB
58 MinifyJS — mean: 51.06 ms max: 54.54 ms rate: 19.59 /s output size: 212 KiB
59extensions/VisualEditor/lib/ve/dist/visualEditor-rebase.js (b1912746; 2.32 MiB)
60 JavaScriptMinifier — mean: 2036.99 ms max: 2390.93 ms rate: 0.49 /s output size: 1.19 MiB
61 MinifyJS — mean: 449.00 ms max: 513.84 ms rate: 2.23 /s output size: 1.23 MiB
62extensions/VisualEditor/lib/ve/dist/visualEditor.js (65ec015e; 2.19 MiB)
63 JavaScriptMinifier — mean: 2072.30 ms max: 2394.22 ms rate: 0.48 /s output size: 1.08 MiB
64 MinifyJS — mean: 429.17 ms max: 457.11 ms rate: 2.33 /s output size: 1.13 MiB
65extensions/Wikibase/client/data-bridge/dist/data-bridge.app.js (6615f253; 209 KiB)
66 JavaScriptMinifier — mean: 443.62 ms max: 499.58 ms rate: 2.25 /s output size: 207 KiB
67 MinifyJS — mean: 59.62 ms max: 62.91 ms rate: 16.77 /s output size: 209 KiB

Thanks for that! Seems to work great - merged!

Krinkle renamed this task from Upgrade ResourceLoader's JS Minifier to support ES6 to Upgrade ResourceLoader JS minifier to support ES6.Feb 10 2021, 7:55 PM

Digging into this further, @Krinkle and I found that, because MinifyJS uses regexes, it can't successfully tell whether / is a division operator or the start of a regex, even with reasonable (non-contrived) inputs. For example,

let num = (foo + bar) / baz, rx = /.global(ly)? unique(ness?)/;
// incorrectly minifies to
let num=(foo+bar)/ baz, rx = /.global(ly)?unique(ness?)/

Also, template strings can be nested (admittedly this is a more niche use case), and that also results in incorrect minification:

console.log( `foo${` hello + there `}bar` );
// incorrectly minifies to
console.log(`foo${` hello+there `}bar`);

I believe both of these issues can't be solved without moving away from regexes in favor of a state machine. Since JavaScriptMinifier uses a state machine, I decided to work on adding ES6 support to it.

Change 664700 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/libs/Minify@master] Add ES6 support

https://gerrit.wikimedia.org/r/664700

Change 664700 merged by jenkins-bot:
[mediawiki/libs/Minify@master] JavaScriptMinifier: Add ES6 support

https://gerrit.wikimedia.org/r/664700

Change 670306 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/vendor@master] Update wikimedia/minify to v2.2.0

https://gerrit.wikimedia.org/r/670306

Change 670307 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] Update wikimedia/minify to v2.2.0

https://gerrit.wikimedia.org/r/670307

Change 670306 merged by jenkins-bot:
[mediawiki/vendor@master] Update wikimedia/minify to v2.2.0

https://gerrit.wikimedia.org/r/670306

Change 670307 merged by jenkins-bot:
[mediawiki/core@master] Update wikimedia/minify to v2.2.0

https://gerrit.wikimedia.org/r/670307

Krinkle assigned this task to Catrope.
Krinkle edited projects, added Performance-Team; removed Performance-Team (Radar).
Krinkle moved this task from Inbox to Doing: Goal-oriented on the Performance-Team board.

I think there’s some subtle bug in the new minifier that’s preventing WikibaseLexeme JavaScript from loading correctly – there’s no error in the console or anything, but parts of the UI don’t initialize with the new minifier, except when the page is loaded in debug mode (which I assume bypasses the minifier). See T277061: Many WikibaseLexeme tests suddenly failing in quibble-vendor-mysql-php72-{selenium,noselenium}-docker.

Change 593934 abandoned by Jforrester:
[mediawiki/core@master] [DNM] resourceloader: Replace JavaScriptMinifier with Minify

Reason:
We built our own, in the end.

https://gerrit.wikimedia.org/r/593934

Change 411387 abandoned by Jforrester:
[mediawiki/core@master] [DNM] Benchmark comparing JavaScriptMinifier and MinifyJS

Reason:
We built our own, in the end.

https://gerrit.wikimedia.org/r/411387

Change 672487 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Update wikimedia/minify to 2.2.1

https://gerrit.wikimedia.org/r/672487

Change 672487 merged by jenkins-bot:
[mediawiki/core@master] Update wikimedia/minify to 2.2.1

https://gerrit.wikimedia.org/r/672487