[00:05:51] TimStarling: domEval is inherently slower, I doubt it was ever faster than any other form of eval. I'm guessing there may've been other PRs about some kind of compat or something. [00:06:13] My comments are about network responses indeed. mw.loader.store eval will always be synchronous and on the spot. [00:07:27] E.g. even in today's version, I would expect compilation to happen at a more desirable point (i.e. not main thread) if we serve mw.loader.implement(…, ( function() {} ), …) with extra needless parenthesis, to signal to the engine that this is gonne be invoked just once, and very soon. [00:08:27] my plan is to bring https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945005 out of the WIP state [00:09:01] I am verifying that patch in firefox [00:09:44] then I will go over to the module serialization branch https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945490 [00:10:30] TimStarling: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945688/1/resources/src/startup/mediawiki.loader.js [00:10:38] I will fiddle with it a bit more to see if there is anything I can do to improve it [00:10:39] note that we do need global for string module eval [00:10:55] new Function will not be global. [00:11:26] yeah, I ddi that didn't I? [00:11:53] you are submitting your patch almost a day after I did this work and subscribed you [00:11:54] oh, I see. you named the non-global eval as globalEval. [00:12:13] I'm just uploadng a patch I had locally to show you, I was testing something else locally yesterday [00:12:16] ignore the patch :) [00:12:54] but even if there is nothing obvious I can do, with PS2 it is only 6ms extra which is probably acceptable [00:13:30] PS2 of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/945490 [00:15:02] I may try eval naming, on top of that patch [00:15:08] and then source maps [00:17:40] TimStarling: I'd try the impl(Fn) as literally calling implement() with parameters, and having that function and the scriptFn wrapped in parens; then as main benchmark, something like Special:RecentChanges (which has a large JS payload) with $wgResourceLoaderStorageEnabled=false to ease repeat testing. [00:20:33] RC is larger than VE? [00:20:45] you know VE was my previous test case [00:25:07] That'll do :) [00:25:32] I would like to have something truly large, like megabytes [00:26:32] one of the blog posts I read yesterday mentioned Facebook loading 5MB or something [00:27:13] I would like it from the point of view of profiling noise, maybe our users would not like it if we built Wikipedia like that [00:27:54] in Firefox with MW master I am seeing a bytecode cache hit in response to domEval(), performance seems to be better than chromium as a result [00:28:57] localStorage.clear(); load https://en.wikipedia.org/w/index.php?title=Roman_Candles_(1920_film)&action=edit; shows me 3.3MB uncompressed JS [00:29:11] That's VE for me btw [00:29:41] with the main two being 1.7MB and 1.1M [00:30:34] TimStarling: yeah, I recall reaching out to Chrome at some point whether they could key their bytecode cache beyond just urls, but their's is tied more closely to the network layer. [00:31:02] Chromium integrates some kind of snapshotting which is actually a bit more than a bytecode cache. [00:31:38] some kind of hybrid between bytecode and VM, since they try to only store what the script "does" so as to not have to re-do it. [00:31:57] This of course only if the script odesn't have side-effects at run time. [00:32:12] firefox profile for VE force reload on MW master: https://share.firefox.dev/3DMcXXm [00:32:27] which we mostly avoid, but there's still plenty that do. We generally encourage $() or mw.rIC() for that. [00:34:00] TimStarling: it's pretty small, but that does contain a asyncEval chunk, suggesting it has localStorage cache hits. [00:34:20] not sure if you wanted to measure store or load.php perf [00:38:46] first job is domEval() so I am measuring store cache hit performance [00:39:57] I see the "precompiled execute in global" now. [00:40:08] Cool, I wasn't sure what to look for. That's pretty cool indeed. [00:42:29] RE: 5MB, note that individual modules larger than 100 kB are not stored in localStorage given the size limit on localStorage. so most of the large VE modules always come from a URL (browser cache or network). [00:42:52] which we given their own RL cache group so that they force their own url, and thus are likely a cache hit in the browser and not mixed with random other modules. [00:43:49] which is not really a compromise in perf, it's faster at that size, but does require some coordinating with RL module groups etc, but something to keep in mind. [00:44:10] I though you were measuring load.php responses because of that. [00:45:58] I was looking at the wrong thing initially [00:46:18] if you just search for domEval then you get to the right thing [00:46:44] and then you see that the larger call is 41ms and it's doing parsing, it's not a cache hit [00:48:32] the first call is at about 2.1s in the timeline and the second is around 3.0s [00:48:51] still faster than chrome's 59ms [00:50:34] domEval -> … -> js::frontend::Parser::globalBody [00:51:15] right, that's compiling presumably from scratch. The precompiled one is the caller, so I guess that's the startup.js response [00:51:22] (ie.. where mw.loader is defined) [00:51:55] maybe from a previous page load, or from the previous cache hit in the same response? don't know. or because it was eagerly compiled at some point [00:53:15] gotta go now, but anyway, happy to review what you come up with tomorrow. I'm still a bit puzzled at times with Firefox very browser-level focussed profiler now that they've deprecated "the normal" one it seems. Takes quite a few steps just to get to the right tab and process. On the other hand, for stuff like this, it's super quick to get to the underlying native bits, which is much harder to get at in Chrome. [00:54:22] it's basically chrome://tracing/ without the Devtools version. [00:55:18] they used to have a perf tab like Chrome in Firefox devtools, but they got rid of it after launchiung the Firefox Profiler [10:54:47] any performance difference between domEval and Function('...') could be due to the difference in exception handling [10:55:36] with domEval() the try / catch did nothing, as Tim notes in the commit message [10:59:50] RL was protected from unhandled exceptions in modules because the modules were executing in a separate execution context [11:02:41] but now module code actually executes in the try / catch [11:03:43] (IIRC, that was also the case when the try / catch was originally written, since it was using eval()). [11:07:59] there are dire warnings about the performance cost of try / catch on the web but they're all a decade old [11:09:49] it probably doesn't matter, but I'm just saying, if there _is_ an observed performance difference to account for, this could be a factor [11:15:03] also if for whatever reason you end up sticking with domEval() I wonder if it could be optimized by having it reuse a single