Reduce complexity of Wikibase JS module registration
Closed, ResolvedPublic

Description

Patch-For-Review:


Looking at flame graphs and overall code complexity, there's quite a bit of complexity in how Wikibase registers its JavaScript modules.

entrypoint=extensions/Wikibase/client/WikibaseClient.php

  • lib/WikibaseLib.php
    • $wgResourceModules = array_merge ...
    • lib/resources/Resources.php
      • return array( .. ); preg_match(,,$remoteExtPath)
  • view/WikibaseView.php
    • view/init.mw.php
      • view/resources.php
        • $wgResourceModules = array_merge ...
          • view/lib/Resources.php
            • return array( .. )
              • view/lib/resources/wikibase-data-model.php
                • view/lib/resources/wikibase-data-model/resources.php
                  • return array( .. ); preg_match(,,$remoteExtPath)
              • view/lib/resources/...
                • ...
          • view/resources/Resources.php
            • return array( .. )
              • view/resources/jquery/resources.php
                • return array( .. ); preg_match(,,$remoteExtPath)
              • view/resources/...
                • ...
      • wgHooks / ResourceLoaderRegisterModules
        • preg_match(,,$remoteExtPath)
        • $rl->register( array )
    • view/resources/Resources.php
  • $wgResourceModules = array_merge ...
    • client/resources/Resources.php
      • return array( .. ); preg_match(,,$remoteExtPath)

It seems the the main reason for all this complexity and indirection is that these used to be separate MediaWiki extensions where the extensions/ path specified for remoteExtPath had to vary between "Name-of-micro-extension", "Wikibase" and "Wikidata".

However, as far as I can tell it will only ever be "Wikibase", given T173818 has been resolved.

Questions:

  • Can all this be simplified to simple php files that return a static array with no preg_match complexity?
  • Can we merge them upwards so that we only have a handful of these files rather than a separate one for each file that used to be its own Git repo? At the very least it'd be nice to reduce from 62 files to less than 10, or even just 2? (One for Client, one for Repo). Note that this should not hinder future potential to publish these as separate npm packages, given that those packages will not need MediaWiki-PHP handling, and those publications can happen even a monorepo sub directory.

Related:

Krinkle created this task.Mar 12 2018, 10:53 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2018, 10:53 PM
Addshore added a subscriber: WMDE-leszek.

I remember working on these regular expressions:

preg_match( '+' . preg_quote( DIRECTORY_SEPARATOR ) . '(?:vendor|extensions)'
    . preg_quote( DIRECTORY_SEPARATOR ) . '.*+', __DIR__, $remoteExtPath );

I once made sure they are all identical, and as short and fast as possible. Note the regex searches for the last occurrence of "vendor|extensions", so a match against "…/extensions/A/vendor/B" would result in "/vendor/B".

The meat of these lines of code was the fact that modules could end in either a "vendor" or an "extensions" subdirectory. This was true for the ValueView extension for a long time, and a possibility we wanted to keep open for more components. From todays perspective these preg_match are not needed any more, and can be replaced with static strings, or even removed.

I strongly oppose the suggestion to merge files. This would way to easily result in giant, unstructured blobs of module definitions that are already hard to read and to maintain on their own. The "ResourceModules" section I currently see growing at the end of extensions/WikibaseLexeme/extension.json in the Lexicographical data project is a 400 lines mess – and development on this extension is not even finished yet. I remember we discussed the existence of the resources.php files in the Wikibase codebase years ago with @adrianheine and, as far as I remember, you, @Krinkle. It might be possible to merge a few modules that closely belong together anyway. But I will not support a reduction from 62 files to 2, not even 10.

Change 419333 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Wikibase@master] [WIP] Module definition: Remove dynamic matching

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

Change 419334 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Wikibase@master] [WIP] Module definition: Reduce some indirection in resources files

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

Change 419335 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Wikibase@master] [WIP] Module registration: Keep global var handling central

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

@thiemowmde Sounds like a good point to start. I won't press on the file reduction for now.

I've published three patches to start the conversation. Feel free to take over at any moment as you see fit, e.g. by amending or by forking to a different change-id and abandoning mine.

thiemowmde moved this task from incoming to ready to go on the Wikidata board.Mar 14 2018, 4:51 PM
thiemowmde edited subscribers, added: Lydia_Pintscher; removed: adrianheine.

Thanks for starting these patches, @Krinkle! I continued working on the first two and left some comments for the Wikidata teams to pick up. From my point of view this task is ready. Can you, @Lydia_Pintscher, please triage and prioritize it?

For reference, @Addshore was providing some numbers in addition to the flamegraphs above:

@Addshore wrote:

For index.php Wikibase.php takes 0.67 of full page load time.
For api.php Wikibase.php takes 0.83
For load.php Wikibase.php takes 1.91%

I just looked at another sample and: For load, Wikibase.php is 2.48%

Change 419489 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Wikibase@master] Inline WikibaseView resource files directly accessing global vars

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

Change 419489 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Inline WikibaseView resource files directly accessing global vars

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

Krinkle moved this task from Inbox to Radar on the Performance-Team board.Mar 19 2018, 8:11 PM
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.

Change 419333 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Module definition: Remove dynamic matching

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

Krinkle updated the task description. (Show Details)Apr 9 2018, 7:44 PM

Change 419335 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Merge view/init.mw.php into view/WikibaseView.php

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

Krinkle updated the task description. (Show Details)Apr 16 2018, 7:47 PM

Change 419334 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Reduce indirection in resources files for view/wikibase-{api,data-model}

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

Krinkle updated the task description. (Show Details)

The remaining work here is to apply "Reduce indirection in resources files" to the rest of Wikibase. The above commit (https://gerrit.wikimedia.org/r/419334) was merely an example for one of the modules (view/wikibase-data-model).

Looking much better:

Krinkle closed this task as Resolved.
Krinkle claimed this task.

Change 485577 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Wikibase@master] Use require_once instead of include_once

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

The rest of the problem (the misuse of the module registry with one class-file per module) is tracked at T203696.

Change 485577 merged by Krinkle:
[mediawiki/extensions/Wikibase@master] Remove redundant defined() checks

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