Page MenuHomePhabricator

ResourceLoader source map on localStorage cache hit
Closed, ResolvedPublic

Description

A complication in implementing source map support (T47514) is figuring out how to deliver the source map for a localStorage cache hit, where JS text is written to localStorage and then eval'd on a subsequent request.

Currently mediawiki.loader.store.set() does its own module serialization, putting back together all the pieces of the original mw.loader.implement() call, and reconstructing roughly the same mw.loader.implement() call that is returned by PHP's makeLoaderImplementScript().

When we have a source map feature, the result of module serialization in mediawiki.loader.store.set() would have to exactly match the one used in PHP while constructing the source map, character for character. I think that would complicate future maintenance.

My idea is to instead call mw.loader.implement() with a declarator function that returns an array of arguments, instead of directly calling it with the arguments. Then the function can be serialized, recovering the exact JS code generated by PHP.

Using an arrow function and shortening "implement" to "impl" reduces the bundle size impact to a couple of bytes per module. So for example

mw.loader.implement("ext.graph.render@2uh7o", ... );

would become

mw.loader.impl(()=>["ext.graph.render@2uh7o", ... ]);

I asked @Krinkle what he thought about this idea. He was concerned about compilation being deferred when it was previously done up front. So I investigated that. It turns out to cause the opposite problem.

On a force reload of a local VisualEditor page, zooming in on the flame graph of the largest domEval() call:

baseline ve reload 20230803T105236.png (357×691 px, 20 KB)

There is a lot of DOM overhead, but eventually it just does a single compilation, lasting 24ms. The domEval() is 59ms.

With PS1 of the WIP patch:

closure ve reload.png (359×546 px, 22 KB)

There is the big compilation, this time lasting 23ms, but then there is another 32ms consisting of many small compilations. Overall, domEval() takes 89ms.

I haven't confirmed whether there is an impact on user-visible latency.

Using plain old functions instead of arrow functions, as in PS2 of the patch, the additional compilation is reduced to 6ms with domEval() taking 74ms.

Rebasing on top of function constructor evaluation (gerrit 945005), with plain declarator functions, the largest globalEval() takes 32ms, which is only 1ms more than what I measured on that patch before touching mw.loader.implement(). So I think the overhead of this variant is acceptable.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 945489 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Forwards-compatible mw.loader.impl()

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

Change 945490 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] ResourceLoader: Simplify module serialization

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

Change 945698 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Break up asyncEval() into batches of 30ms

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

Change 945699 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] ResourceLoader: Simplify module serialization

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

Change 945698 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Split up asyncEval() into small batches

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

Change 947503 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Implement source map support for localStorage cache hits

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

Change 945489 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Forwards-compatible mw.loader.impl()

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

Change 945490 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: wrap module definitions in functions

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

Anecdotal comparison for future reference.

Before: master at 31fcbb83c18d417acffc8e82cd28a6d10a0bece9

After: review/tim_starling/2023/rl-source-maps-v2, which is ahead by three changes:

  1. Forwards-compatible mw.loader.impl(). https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945489/11
  2. ResourceLoader: wrap module definitions in functions. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945490/12
  3. ResourceLoader: Simplify module serialization. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945699/15 (72cf4f89d2312dc9379def8f415bd1245700efb5)

On both sides, I did the following:

  • Set $wgResourceLoaderStorageEnabled = false; to consistently measure a single scenario only (the HTTP/browser cache scenario).
  • Load http://localhost:8080/wiki/Special:RecentChanges in Chrome 115.
  • In Devtools, performance, with CPU 6x throttled. Reload it three times. Saving all captures.

The main differences I found:

  1. The startup module's transfer size shrinks slightly, from 10.7kB to 10.6kB.
  2. Before, the first Compile Code task after the large JS response (the one for page modules, not the startup module) is consistently 8.6-8.7ms. After, this decreases to 5.2-5.4ms.
  3. Before, the total self-time for Compile Code (in the Bottom-Up tab) between LoadEvent and end of capture is 150-164ms. After, this seems to increase to 161-181ms.

The ranges overlap though, and there's quite a bit of standard deviation.

I tried changing the code on top of this stack, to add parenthesis around the mw.loader.impl() callback and around the HtmlJsCode function expressions. This seems to increase the intial compilation (outside the critical path, I believe) to about ~13ms, but then decreases the overall compilation cost to 111-129ms.

Change 945699 merged by jenkins-bot:

[mediawiki/core@master] ResourceLoader: Simplify module serialization

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

Change 947914 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@wmf/1.41.0-wmf.20] ResourceLoader: Forwards-compatible mw.loader.impl()

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

Change 947914 abandoned by Tim Starling:

[mediawiki/core@wmf/1.41.0-wmf.20] ResourceLoader: Forwards-compatible mw.loader.impl()

Reason:

wmf.20 is no longer deployed

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

Change 947503 merged by jenkins-bot:

[mediawiki/core@master] Implement source map support for localStorage cache hits

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

Change 963124 had a related patch set uploaded (by Krinkle; author: Krinkle):

[operations/mediawiki-config@master] Remove wgResourceLoaderStorageVersion override

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

Change 963124 merged by jenkins-bot:

[operations/mediawiki-config@master] Remove wgResourceLoaderStorageVersion override

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