Last modified: 2014-02-16 02:44:38 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T60259, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 58259 - Use Function rather than $.globalEval to compile cached JavaScript code
Use Function rather than $.globalEval to compile cached JavaScript code
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
ResourceLoader (Other open bugs)
1.23.0
All All
: Normal normal (vote)
: ---
Assigned To: Ori Livneh
: performance
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-10 06:34 UTC by Ori Livneh
Modified: 2014-02-16 02:44 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Ori Livneh 2013-12-10 06:34:36 UTC
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>.
Comment 1 Gerrit Notification Bot 2013-12-10 06:44:58 UTC
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
Comment 2 Gerrit Notification Bot 2013-12-11 00:31:38 UTC
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
Comment 3 Gerrit Notification Bot 2013-12-11 00:32:57 UTC
Change 100542 merged by jenkins-bot:
Module storage: randomly choose between Function and $.globalEval

https://gerrit.wikimedia.org/r/100542
Comment 4 Krinkle 2013-12-11 00:44:33 UTC
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.
Comment 5 Krinkle 2013-12-11 00:47:55 UTC
(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.
Comment 6 Andre Klapper 2014-02-14 13:14:51 UTC
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?
Comment 7 Gerrit Notification Bot 2014-02-15 23:34:00 UTC
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
Comment 8 Bartosz Dziewoński 2014-02-15 23:37:48 UTC
The patch was reverted later and AFAIK was never deployed anywhere in the end. Ori, status update please? :)
Comment 9 Ori Livneh 2014-02-16 02:44:38 UTC
new Function() is 3x-4x slower than $.globalEval on some browsers, so it's a non-starter. Thanks for poking.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links