Page MenuHomePhabricator

Use source urls in mw.loader.store
Closed, DeclinedPublic

Description

ResourceLoader uses localStorage to cache module code; cached code is loaded via eval(), which means that the browser cannot associate it with a source URL. This will be eventually fixed by source maps, but as a temporary measure, we could use sourceURL directives to maintain that association.

14:29 <Krinkle> we can construct them client-side and put anything in there, even wgVersion, in addition (or instead) of module.version
14:30 <Krinkle> e.g. vm://mw-loader-store/{wgVersion}/{module}.js
14:30 <Krinkle> I haven't tested browser support on that though

Event Timeline

Tgr created this task.Feb 24 2015, 3:32 AM
Tgr updated the task description. (Show Details)
Tgr raised the priority of this task from to Needs Triage.
Tgr added subscribers: Tgr, Krinkle.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 24 2015, 3:32 AM
Aklapper triaged this task as Normal priority.Feb 24 2015, 9:34 AM

Change 156544 had a related patch set uploaded (by Krinkle):
[WIP] mw.loader.store: Set virtual sourceURL in the globalEval calls

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

Krinkle lowered the priority of this task from Normal to Low.Mar 11 2015, 11:32 PM
Krinkle set Security to None.
Tgr added a comment.Apr 9 2015, 2:49 PM

From UploadWizard error logging data (T95527):

mysql:research@analytics-store.eqiad.wmnet [log]> select count(*) total, sum(event_url like '% > eval') `> eval`, sum(event_url like '%Special:UploadWizard%') `Special:UploadWizard`, sum(event_url = 'undefined') undefined, sum(event_url = '') empty, sum(event_message like 'Script error%') cors from UploadWizardExceptionFlowEvent_11717009; 
+-------+--------+----------------------+-----------+-------+------+
| total | > eval | Special:UploadWizard | undefined | empty | cors |
+-------+--------+----------------------+-----------+-------+------+
|  3639 |    173 |                  694 |         9 |  2667 |  321 |
+-------+--------+----------------------+-----------+-------+------+

so ~97% of error locations are crippled, and CORS limitations only account for 10% of that. The proportions are probably going to be different for reader-facing pages, and function names in full stack traces will give a little more hint of where an error comes from, but it's clear that we need either source maps or sourceURLs to make good use of error logs.

Tgr added a comment.EditedApr 9 2015, 10:02 PM

Played around a bit with the latest version of the patch in latest Chrome and Firefox (with and without Firebug)( on Linux. Chrome displays the generated URL on the console and uses it in stack traces; Firefox instead shows a rather useless [jQuery module name] > eval. The source tab becomes more reasonable - Chrome and Firebug show the module code under the provided link (without sourceURL, FF shows it with the > eval URL and Chrome does not show it at all). There are some small changes in behavior (e.g. clickthrough from console link to source breaks in Chrome after navigating away from the page) but nothing significant.

In short, this will fix error logging on Chrome (and leave it useless but not any worse than it was on Firefox) without having any significant negative impact on Chrome/FF manual debugging.

The presentation of such errors on the Sentry interface is not so great as it strips out the query string, so the everything ends up attributed to load.php, but that can probably be fixed with a plugin (or some nasty hack like using /w/load.php/<module>?... URLs).

Change 156544 abandoned by Krinkle:
[WIP] resourceloader: Set sourceURL in eval calls for mw.loader.store

Reason:
See T90524.

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

Krinkle removed Krinkle as the assignee of this task.Nov 18 2016, 3:31 AM

Moving back to backlog for now.

This is blocked because we're evaluating the cached module implementations from localStorage in one large globalEval() call. The sourceURL feature in browser's developer tools only supports 1 such comment per evaluated source. As such, unless we do separate evals for each chunk, we can't add these debug urls.

Originally the plan in T127328 (to make cache-eval async) also involved slicing it up in separate chunks (based on mw.requestIdleCallback's 50ms deadline). This would have the benefit of not freezing the user's device for an extended period of time during which no input can be received. The slicing would be based on the device's speed. The faster js execution is, the fewer chunks.

However this aspect of the "async cache-eval" change was reverted as it regressed the mwLoadEnd metric significantly for the p95/p99 lines. And since we don't currently have any metrics that proved this suspected benefit, we couldn't accept this regression.

We should probably figure out a way to measure "interactiveness", or more specifically, to measure how much time we spend in globalEval, to see what the.long tail looks like. If that really involves devices executing JS synchronously for say 1 full second or more, then that would be a metric we can se improve when we slice the work up. The fact that mwLoadEnd will be later would be acceptable.

Anyway, that's for a later time. Until then, this is shelved.

Using the following patch to test locally using MediaWiki-Vagrant.

before (large batch - current master)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1688,8 +1688,14 @@
+                   var i;
                    try {
+                       console.log( 'asyncEval: ', implementations.length, 'implementations' );
+                       console.time( 'globalEval' );
                        $.globalEval( implementations.join( ';' ) );
+                       console.timeEnd( 'globalEval-for' );
                    } catch ( err ) {
after (chunked)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1688,8 +1688,14 @@
+                   var i;
                    try {
-                       $.globalEval( implementations.join( ';' ) );
+                       console.log( 'asyncEval: ', implementations.length, 'implementations' );
+                       console.time( 'globalEval-for' );
+                       for ( i = 0; i < implementations.length; i++ ) {
+                           $.globalEval( implementations[ i ] );
+                       }
+                       console.timeEnd( 'globalEval-for' );
                    } catch ( err ) {

The test page was MediaWiki-Vagrant's Main Page. On a repeat visit, 59 modules are loaded from cache in the top queue. Later 3 more modules from a lazy-loaded script (ULS webfonts data). This gives a nice sample set to see if there is a startup cost or other overhead that may make the results differ from smaller vs larger batches.

Results are from 6 repeat visits before, and 6 repeat visits after. Sorted in ascending order.

Chrome 57 canary on OSX

MacBook Pro, CPU throttle: 5x.

Before:

asyncEval: 59 implementations
globalEval: 268ms, 270ms, 288ms, 297ms, 316ms, 319ms

asyncEval: 3 implementations
globalEval: 16.4ms, 18.1ms, 19.3ms, 19.8ms, 21.9ms, 22.3ms

After:

asyncEval: 59 implementations
globalEval-for: 299ms, 300ms, 303ms, 339ms, 352ms, 380ms

asyncEval: 3 implementations
globalEval-for: 14.5ms, 16.4ms, 16.6ms, 17.7ms, 23.9ms, 26.8ms

For the larger batch it seems to add 15-60ms. For the small batch it seems to mostly unchanged (between 2ms faster and 4ms slower).

Chrome 54 stable on OSX

MacBook Pro, CPU throttle: 5x.

Before:

asyncEval: 58 implementations
globalEval: 248ms, 257ms, 263ms, 277ms, 279ms, 320ms

asyncEval: 3 implementations
globalEval: 15.7ms, 16.2ms, 16.9ms, 18.6ms, 19.4ms, 19.5ms

After:

asyncEval: 59 implementations
globalEval-for: 286ms, 321ms, 331ms, 347ms, 371ms, 374ms

asyncEval: 3 implementations
globalEval-for: 17.3ms, 18.5ms, 20.2ms, 20.4ms, 20.8ms, 24.5ms

Larger batch: 38-54ms slower. Small batch: 2-5ms slower.

Safari 9.1.3

MacBook Pro. (No throttle option available)

Before:

asyncEval: 58 implementations
globalEval: 35.537ms, 37.123ms, 37.394ms, 38.559ms, 38.674ms, 43.358ms

asyncEval: 3 implementations
globalEval: 2.039ms, 2.164ms, 2.303ms, 2.317ms, 2.780ms, 3.738ms

After:

asyncEval: 58 implementations
globalEval-for: 35.752ms, 37.589ms, 39.127ms, 40.410ms, 42.454ms, 43.345ms

asyncEval: 3 implementations
globalEval-for: 1.882ms, 1.978ms, 2.274ms, 2.397ms, 2.443ms, 3.100ms

Larger batch: unchanged (between 0.2ms slower and 0.01ms faster). Small batch: 0.2ms-0.6ms faster.

Firefox has mw.loader.store disabled. IE 11 has issues (T152180).

Krinkle closed this task as Declined.Jun 20 2018, 10:05 AM

Can't be done performantly. See T47514 for something that could address the underlying use case.