Page MenuHomePhabricator

Investigate if Cite ResourceLoader module for logging should be merged
Open, Needs TriagePublic


T231529: Understand current referencing behavior as baseline for ReferencePreviews introduces a new MediaWiki-ResourceLoader module. The Performance-Team asked us to avoid this. As far as I (@thiemowmde) understand the concern, it goes like this:

  1. The Cite extension is such an essential part of our Wikimedia infrastructure, we can think of it as being part of core. We do ship it everywhere.
  2. Same for Analytics-EventLogging.
  3. This means the logging code in Cite will be active everywhere.
  4. This means the fact it's a separate MediaWiki-ResourceLoader module is useless on our Wikimedia cluster.
  5. Merging it into another module get's rid of the few bytes that are needed to register the module.
  6. But we need to add more code to not activate any logging in case Analytics-EventLogging is not available.

The question we need to answer is: what is more costly (for us vs. for 3rd parties)?

Event Timeline

Krinkle added a comment.EditedOct 5 2019, 2:29 AM

Yes, this should not be its own bundle. It has no unique use case of being needed in a contexts where there isn't at least one other thing always present in the same contexts as well.

In this case, the most natural bundle to ship this feature with is ext.cite.ux-enhancements, which is loaded from the same code 1 line up. See Gerrit note.

  1. But we need to add more code to not activate any logging in case EventLogging is not available

This is the only mentioned downside which I believe is a misunderstanding. Modules can be composed dynamically, using the same dynamics the the module is currently registered. There is no need for additional client-side code. Best of both :) There are several extensions doing this already, let me know if you'd like me to find an example.

what is more costly?

There is a concrete answer, but ultimately it is about balance. My general rule of thumb is to bundle by default. And only after you find there is a performance problem or budget exceeded in terms of bundle size, consider optimising it. At that time module splitting can be one (of several) possible optimisations to choose from - which should be balanced against the various costs it brings.

Worrying about shipping a small amount of unused code is easy. And this feels weird to accept, as it is a seemingly easy thing to address as developer (Just split it, right?). However, perfect fragmentation comes with many hidden costs. (memory, cpu, registry bandwidth, request splitting due to url size) that can quite easily end up costing more than the data it helps avoid loading. In those cases we'd make a zero-sum effort for third parties, and have Wikipedia pay double.

Krinkle moved this task from Inbox to Radar on the Performance-Team board.
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Perf recommendation on the Performance-Team (Radar) board.
awight added a comment.Oct 7 2019, 8:25 AM

Thank you for taking the time to look at this, I think I finally get it. I've been concerned about creating two versions of the ext.cite.ux-enhancements module, with and without eventlogging, which would break caching, but this isn't a valid concern because eventlogging is either enabled or not, and that will only change on very long time scales if ever. So if eventlogging is present, I can safely bundle our class with the existing module.

I'll make the change and post to this task.

Change 541200 had a related patch set uploaded (by Awight; owner: Awight):
[mediawiki/extensions/Cite@master] Bundle tracking with another RL module

Change 541200 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Bundle tracking with another RL module

Change 543756 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Fix rebase mistake in extension.json

Change 543756 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Fix rebase mistake in extension.json