Page MenuHomePhabricator

Move Echo site-level data from MakeGlobalVariablesScript to a module
Closed, ResolvedPublic

Description

For performance reasons, static data should go in ResourceLoaderGetConfigVars, not MakeGlobalVariablesScript.

The key difference is that static data (ResourceLoaderGetConfigVars) goes in the startup module and is loaded once (until the startup module is re-generated/cache expires, then it needs to be downloaded again).

MakeGlobalVariablesScript data is in the source code of every page, so it should only be used when the data is request-specific. Generally, this may be specific to the page, user/username, or other aspects of the request.

wgEchoOverlayConfiguration['max-notification-count'] is static (unlike notification-count), so it should ideally be broken into a separate static variable.

ResourceLoaderGetConfigVars and wgEchoConfig are also static. However, there's a complication that wgEchoConfig is set differently when testing. This exception should be dropped so it can be static, if possible.

The comments should also be updated so it's clear to people which one to use.


See also: T221151: Move Echo page-level data to something async (wgEchoInteractionLogging, wgEchoEventLoggingVersion)

Details

Reference
bz71539

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:55 AM
bzimport set Reference to bz71539.
bzimport added a subscriber: Unknown Object (MLST).
matthiasmullie triaged this task as Normal priority.Dec 10 2014, 5:05 PM
matthiasmullie set Security to None.
Mattflaschen-WMF raised the priority of this task from Normal to High.Mar 21 2015, 4:04 AM

Per description, this will also fix issue mentioned in https://gerrit.wikimedia.org/r/#/c/198459/ .

Beware with bloating the startup module though. The data may be static but it is also served to all users, user groups, pages, namespaces and devices.

If this is currently output unconditionally at least it won't make it much worse, but if this data is significant in size and/or not needed on every page everywhere, it may make more sense to provide it as a data module in Echo directly instead of doing everything via the global mw.config transport layer.

See VisualEditorDataModule and LanguageDataModule as example.

Mattflaschen-WMF renamed this task from Static data should go in ResourceLoaderGetConfigVars, not MakeGlobalVariablesScript to Static data should go in ResourceLoaderGetConfigVars or data module, not MakeGlobalVariablesScript.May 14 2015, 4:12 AM
Legoktm lowered the priority of this task from High to Lowest.Jul 6 2015, 8:07 AM

These variables are only used by logged-in users (on nearly every pageview), so serving them to all users wouldn't be very useful.

Legoktm moved this task from Backlog to Needs plan on the Notifications board.Jul 6 2015, 8:07 AM
Krinkle closed this task as Declined.Oct 26 2015, 4:29 PM
Krinkle claimed this task.

Per previous comments, contrary to opening post, I believe moving this data to startup would make performance worse, not better. If the repeated download of this data is a problem, move it its own module. Not the startup module.

Mattflaschen-WMF renamed this task from Static data should go in ResourceLoaderGetConfigVars or data module, not MakeGlobalVariablesScript to Static data should go in data module, not MakeGlobalVariablesScript.Oct 26 2015, 6:50 PM
Mattflaschen-WMF reopened this task as Open.

Do you think it makes sense to do it in a data module?

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 26 2015, 6:50 PM
Krinkle removed Krinkle as the assignee of this task.Oct 27 2015, 4:41 AM

If the data is the same for all users/pages, and large "enough"; using a data module makes sense yeah.

Consider doing it without introducing a new module though but rather extend a relevant module that is already loaded in the same semantic context and extend it with non-file data. Otherwise adding a new module as dependency is fine too.

DannyH removed a subscriber: DannyH.Nov 6 2015, 12:56 AM
Krinkle removed a subscriber: Krinkle.Oct 26 2016, 12:23 AM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptOct 26 2016, 12:23 AM
Krinkle renamed this task from Static data should go in data module, not MakeGlobalVariablesScript to Move Echo site-level data from MakeGlobalVariablesScript to a module.Jul 4 2019, 7:19 PM
Krinkle updated the task description. (Show Details)
Krinkle removed a subscriber: wikibugs-l-list.

I noticed in last weeks' deployment that the Echo extension has added another site-level data key to all page views:

startup
- "wgVersion": "1.34.0-wmf.10",
+ "wgVersion": "1.34.0-wmf.11",+ "wgEchoPollForUpdates": 0,
Quiddity removed a subscriber: Quiddity.Jul 4 2019, 8:19 PM

Change 520660 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/extensions/Echo@master] Use packageFiles instead of startup module for config vars

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

JTannerWMF added subscribers: Catrope, JTannerWMF.

Would you look at that @Catrope, created a patch, in code review this goes.

I accidentally dropped the reference to this task from the patch, but it's now merged.

Etonkovidova closed this task as Resolved.Wed, Jul 31, 7:45 PM
Etonkovidova claimed this task.