Page MenuHomePhabricator

Use Function rather than $.globalEval to compile cached JavaScript code
Closed, DeclinedPublic

Description

The scope of eval()'d code is determined by the scope in which it was created and by whether or not the code uses the ES5 strict mode pragma. This makes it so tricky to analyze that at least one JavaScript engine, V8, declines to optimize it altogether. The same is not true of JavaScript source compiled via the Function object constructor, since such code always executes in global scope.

We should test if Function is faster and use it if so.

See this Wikitech-l thread for context: http://www.gossamer-threads.com/lists/wiki/wikitech/414263#414263.


Version: 1.23.0
Severity: normal

Details

Reference
bz58259

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:37 AM
bzimport set Reference to bz58259.

Change 100542 had a related patch set uploaded by Ori.livneh:
Module storage: randomly choose between Function and $.globalEval

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

Change 100721 had a related patch set uploaded by Ori.livneh:
Module storage: randomly choose between Function and $.globalEval

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

Change 100542 merged by jenkins-bot:
Module storage: randomly choose between Function and $.globalEval

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

Note that globalEval doesn't use plain eval(), it uses various tricks to ensure it is in the neutral and global scope (using the context from the inner closure, which is bound to the global object).

Whether it optimises or not is a separate question, but if globalEval is affected by either MediaWiki's or jQuery's closure, that'd be an upstream jQuery bug. Please make sure that is reported upstream (if not already).

For performance, we can indeed measure both. Make sure though to take into account that new Function executes in a different way than eval(). The most significant difference is the returned value (which we don't care about), but there are other parser context differences that might affect us. Such as exception handling (we need to know about exceptions so that we can put modules in the error state).

As long as it is only given single, plain, unwrapped and balanced calls to mw.loader.implement, it should be fine though.

(In reply to comment #4)

Whether it optimises or not is a separate question, but if globalEval is
affected by either MediaWiki's or jQuery's closure, that'd be an upstream
jQuery bug. Please make sure that is reported upstream (if not already).

So, assuming jQuery did properly scope globalEval, that means V8 has room for optimisation here and do the same thing to globally-scoped eval() as with e.g. new Function. V8 is in a nice healthy and quick iteration through Chrome, so be careful not to optimise for the optimiser, that can and will change. Generally best to optimise for good patterns, and have engines optimise for good code naturally.

I'm looking forward to your results from js engines in other browsers.

new Function seems like a cleaner and more direct pattern as well, so I'd argue its nicer even if it does not always (yet) perform better.

Ori:
Patch was merged into git master - I guess https://gerrit.wikimedia.org/r/#/c/100721/ for 1.23wmf5 can be canceled and this ticket closed as RESOLVED FIXED?

Change 100721 abandoned by Bartosz Dziewoński:
Module storage: randomly choose between Function and $.globalEval

Reason:
This is obviously not getting merged. The bugs needs updating with the current status.

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

The patch was reverted later and AFAIK was never deployed anywhere in the end. Ori, status update please? :)

new Function() is 3x-4x slower than $.globalEval on some browsers, so it's a non-starter. Thanks for poking.