Page MenuHomePhabricator

ResourceLoader LESS cache should vary on wiki-global vars (aside from module own vars)
Closed, ResolvedPublic

Description

We ran into this problem on the Hydra Wiki Platform since ResourceLoader shares CSS caches across wikis where it can.(Which is great!) There is the ability to set Less variables for the wiki's skin CSS through the administrator control panel. However, while ResourceLoader would generate the proper CSS the compileLessFile() function in FileModule would get a blank set of variables for hashing into the cache key since it was not pulling from the context. This resulting in wikis fighting each other trying to rebuild slightly different CSS on each page load.

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

Note: I did not write this patch. Another member of the team here wrote it.

Event Timeline

Alexia created this task.Apr 10 2018, 10:40 PM
Restricted Application added a project: Performance-Team. · View Herald TranscriptApr 10 2018, 10:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Alexia updated the task description. (Show Details)Apr 10 2018, 10:42 PM

Thanks!

The global Less variables from the ResourceLoader instance were omitted intentionally from the LESS cache hash, however, there's no doubt a problem here that we need to address. So let's investigate why they were omitted, and how we can make this work.

At the very least this will need to compute the hash based on both $this->getLessVars and the global ones. Using only one them will either cause the problem you report (bad sharing of cache between wikis with different globals), or a new problem (bad sharing cache among different contexts of the same module within a wiki, e.g. language/skin variation) - which would break things like EditToolbar module, and various other things that vary based on $context.

With regards to why it was omitted from the LESS cache, the only global Less variable that exists by default is deviceWidthTablet, which was deprecated. Extensions can extend this, and some tarball-bundled and wmf-deployed extensions use to do so, but never in a way that was dynamic or variable per wiki.

While in theory they could, I never imagined they would, and a proposal to do so would likely have been rejected for performance reasons. Allow me to explain. The LESS cache is fairly internal. A change resulting in that cache key computing differently, would not result in end-users seeing new values. Instead, for end-users to notice, something that is tracked by the module needs to change. E.g. something tracked by getVersionHash/getDefinitionSummary.

The FileModule::getDefinitionSummary only tracks the module-level getLessVars, not the global ones. And for good reason, as otherwise every change to a global less variable would invalidate all internal and external FileModule caches because a module might use the variable.

Due to this and other problems with global variables, the feature was deprecated in 2016 (MW 1.27), see also T140804. We kept it for back-compat, but with the documented caveat (in DefaultSettings.php) that changes do not propagate through caches. However, I did not realise, that it could vary by wiki, in which case it would cause worse problems, as reported here.

My recommendation is two-fold:

  1. Migrate from extending wgResourceLoaderLESSVars, to instead extending FileModule::getLessVars (e.g. in a subclass), and use that class for modules that use these variables (presumably the Hydra skin module). Doing this will immediately result in a reduced footprint of the APC cache for LESS, and increased cache-hit ratio – on installations with the patch from https://gerrit.wikimedia.org/r/#/c/425443/ applied, as it would mean that other modules no longer vary by wiki.
  1. Keep the caveat of not actively purging external caches for modules based on wgResourceLoaderLESSVars, but fix the internal LESS cache to vary on wgResourceLoaderLESSVars as well, for correctness – until the feature is removed in MediaWiki 1.32. This is essentially your patch.
Krinkle renamed this task from ResourceLoader needs to pull Less variables from the context otherwise the generated cache key is incorrect. to ResourceLoader LESS cache should vary on wiki-global vars (aside from module own vars).Apr 11 2018, 12:41 AM
Krinkle triaged this task as High priority.
Krinkle moved this task from Inbox to Accepted: Bugs on the MediaWiki-ResourceLoader board.

Change 425443 had a related patch set uploaded (by Krinkle; owner: Alexia):
[mediawiki/core@master] resourceloader: Include global LESS variables in LESS cache key

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

I will add that task to Hydra's internal Jira to move to extending the Module set of classes for this.

Imarlier moved this task from Inbox to Doing on the Performance-Team board.Apr 16 2018, 8:04 PM
Krinkle moved this task from Doing to Blocked on the Performance-Team board.May 21 2018, 8:26 PM

Change 425443 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Include global LESS variables in LESS cache key

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

Krinkle closed this task as Resolved.May 28 2018, 1:13 PM
Krinkle claimed this task.
Krinkle moved this task from Blocked to Doing on the Performance-Team board.
Krinkle removed a project: Patch-For-Review.