Page MenuHomePhabricator

Improve performance of key/value maps used in mw.loader
Closed, ResolvedPublic

Description

In JavaScript, all objects inherit from the same base class. While typical use of objects is through inheritance of a consistent "shape", when using an object as a key/value map, the shape changes constantly. V8 attempts to detect this by foregoing its "implicit class" (or "hidden class") model in favour of a hashtable. However it seems to be failing (when tracing v8 through Chrome, it's optimiser repeatedly warns about this but never seems to kick in).

Main offenders of key/value "objects":

  • mw.loader.register() when augmenting with the registry object.
  • mw.loader#execute() when calling mw.messages.set() to augment its values object.
  • mw.loader#sortDependencies() when augmenting the unresolved object.

As part of the V8/Wikipedia meet-up in SF, it was recommended to use a native ES6 Map or ES6 Set instead (where available). To keep the startup module light I recommend we don't ship a feature-complete polyfill but instead only abstract the minimal feature set for our internal use (not exposed publicly).

Action items:

  • Create private cross-browser interface for Set.
    • Use in sortDependencies().
  • Deprecate mw.Map#values (as mw.config.values, mw.messages.values) - this is the only part of mw.Map that we can't feasibly polyfill.
    • Create private cross-browser interface for Map.
    • Use it inside mw.Map.
  • Deprecate mw.Map. There is no reason for this to be public. And in the last 3 years I've yet to see first use outside mediawiki.js. For most cases, plain objects or arrays suffice. For more elaborate cases, use generic polyfill modules.

Event Timeline

Change 312433 had a related patch set uploaded (by Krinkle):
mw.loader: Use native Set where possible instead of string keys

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

Krinkle triaged this task as Medium priority.Sep 23 2016, 6:37 PM

Change 312557 had a related patch set uploaded (by Krinkle):
resourceloader: Deprecate mw.Map

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

Change 312433 merged by jenkins-bot:
mw.loader: Use native Set where possible instead of string keys

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

Change 313231 had a related patch set uploaded (by Krinkle):
[WIP] resourceloader: Re-implement mw.Map atop native Map w/ fallback

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

Change 312557 merged by jenkins-bot:
resourceloader: Deprecate mw.Map

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

The uses of mw.config.values.wgFoo that "have appeared in the wild" are because of me. I would like to rectify that: https://meta.wikimedia.org/wiki/Steward_requests/Global_permissions#Global_editinterface_for_Nirmos

The deprecation process used here needs work. mw.Map is used in MediaViewer qunit tests (where the change caused T150575: Qunit tests for MultimediaViewer fail with the currently submitted code) for mocking mw.config and a simple grep would have discovered that. It's also apparently used in ImageMetrics, BlueSpiceFoundation, EducationProgram and Wikibase (at a glance, only the first one is broken). Also, removing a public utility class should be mentioned in the release notes.

In general, if a class is used as part of the public interface, it should not be made private IMO.

Change 319478 had a related patch set uploaded (by Gergő Tisza):
Fix broken tests

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

Change 322391 had a related patch set uploaded (by Gergő Tisza):
Fix broken tests

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

Change 322394 had a related patch set uploaded (by Gergő Tisza):
Fix mw.Map instance creation

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

Change 322394 abandoned by Gergő Tisza:
Fix mw.Map instance creation

Reason:
Not needed

Sorry, got confused. The signature did change but as long as it's called with no arguments it should make no difference.

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

In general, if a class is used as part of the public interface, it should not be made private IMO.

If you do intend to keep it private, please work with the BlueSpice/Wikibase maintainers to get it replaced. Either way, the changes should be backported to 1.28.

Change 322395 had a related patch set uploaded (by Gergő Tisza):
Fix broken tests

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

Change 322397 had a related patch set uploaded (by Gergő Tisza):
Fix broken tests

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

The deprecation process used here needs work. mw.Map is used in MediaViewer qunit tests (where the change caused T150575: Qunit tests for MultimediaViewer fail with the currently submitted code) for mocking mw.config and a simple grep would have discovered that. It's also apparently used in ImageMetrics, BlueSpiceFoundation, EducationProgram and Wikibase (at a glance, only the first one is broken). Also, removing a public utility class should be mentioned in the release notes.

mw.Map isn't being removed yet. It's just being deprecated. Not even with warnings at this point. Usage in core, BlueSpiceFoundation, Wikibase, and other extensions are not affected.

The mw.Map(Object) signature was removed indeed. I did a search through all wikis (mwgrep) and Git and must have missed the two callers in MediaViewer and ImageMetrics. Fixed per 789ccdeae6 and e1b31c852b. Thanks.

Even without the mw.Map to ES6 Map conversion (which I expect may happen sometime next year, if at all) - the main issue is resolved with the introduction of StringSet for sortDependencies() as a hot path. Closing as such.

During the V8-Wikipedia meeting earlier this year, we did see mw.messages.set() show up a few times as part of module implementation, but it's negligible in practice and not worth having an issue about. In fact, we had reason to believe it was a V8-specific bug that has since been fixed.