Page MenuHomePhabricator

delayed kicking in of Bridge
Open, HighPublic

Description

Problem:
When clicking the link to open the Bridge too soon after reloading the page I get sent to Wikidata directly. I assume this is because there is a delay between page load and Bridge code kicking in. We should see if we can reduce this or find some other way to solve the issue.

Acceptance criteria:

  • document steps needed to achieve a time between loading of the page (DomContentLoaded) and Bridge kicking in should be less than 2s

timeboxed: 1 day

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 19 2019, 1:01 PM
Lydia_Pintscher triaged this task as High priority.Sep 20 2019, 10:52 AM
Michael added a subscriber: Michael.EditedSep 24 2019, 9:09 AM

Story time notes:

  • If trade-offs are necessary: Let's discuss
  • Can we change the order in which the initial modules are loaded?
  • keep in mind if loading bridge slows down overall page loading
Michael updated the task description. (Show Details)Sep 24 2019, 9:10 AM

One thought I have: the init module is supposed to be small, but dist/data-bridge.init.js currently occupies some 83 KiB. Why do we send it through Babel etc. at all? If it’s not too much code, maybe we should just write it in plain JS, trading some development convenience for a drastic reduction in script size.

I did some small experiments with performance.mark(). (Helpfully, there is already an mwStartup mark emitted by the MediaWiki startup module.)

On my local wiki, it always takes ca. three seconds until even the mwStartup event. This time seems to be dominated by the network request to get the startup module (even when it’s cached, the server needs a while to generate the 304 Not Modified response), and I don’t think we can do anything about it.

If the network cache is disabled, it takes a bit more than two seconds (up to 2½) to get from mwStartup to the beginning of init.ts, and then a bit less than one second from there until the event listeners are added to the links. With network cache, the first time shrinks to about a second and the second one to ca. 100 ms; the first one is still largely due to waiting on the server to return 304 Not Modified, while the second one doesn’t seem to hit the server at all (though I don’t understand why). The time spent in our own code, e. g. to identify the links, is negligible as far as I can tell.

On Beta, custom performance marks aren’t available, but the server is generally faster. However, there’s a suspiciously long gap between the end of the network request for the init module and the beginning of the network request for the app module – slightly over half a second, with or without cache, whereas locally it’s more on the order of 0.3 s. I suppose that might be due to the larger number of things running on Beta in general – are we really running that much synchronous JS in the various modules?

Can we change the order in which the initial modules are loaded?

We probably can with some more-or-less evil hacks (register some hook to reorder $wgResourceModules, register some hook that makes a core module depend on our init module so it gets elevated in the dependency graph, …), but I’m not convinced that’s fair to other modules :)

I see two options to improve performance (independent of each other):

  • Register the click handlers earlier. Currently, we wait for ResourceLoader to finish loading the app module (and wbRepo etc.) before we add event handlers to the bridge links. I see no reason to do this – we can start that load, but then immediately add our event handlers, and await the load within those event handlers if necessary (after stopping native click handling). This should completely remove the second time mentioned above (some 100 ms with cache on my local wiki, about a second without cache), or rather, if the user clicks fast enough they will have to wait for that duration before we begin setting up the dialog. (If we’re smart about it, we can even start setting up the OOUI dialog before waiting for the ResourceLoader to finish, though I’m not sure if that’s worth the effort.)
  • Drastically reduce the size of the init module. This is supposed to be a simple preloader for the real code, and yet it’s more than 80 KiB, 10.5% (before compression) of the main ResourceLoader request (which loads all the non-startup modules at once – ULS data, CentralAuth, Vector, jQuery, OOUI, …) on Beta. I’m still very tempted to rewrite it in plain JS and serve that as a regular old ResourceLoader module – according to find src/mediawiki/ -type f -name '*.ts' -exec cat {} + | wc -l it’s some 300 lines of TypeScript, that’s not the end of the world – but we can also first look into why it’s so bloated, and what kind of code webpack is including in that bundle.

I’m still very tempted to rewrite it in plain JS and serve that as a regular old ResourceLoader module – according to find src/mediawiki/ -type f -name '*.ts' -exec cat {} + | wc -l it’s some 300 lines of TypeScript, that’s not the end of the world –

Correction: the init module also includes some files from src/data-access and a few others, so ca. 700 lines seems to be closer to the real number. (But that’s also including some interfaces and enums.)

but we can also first look into why it’s so bloated, and what kind of code webpack is including in that bundle.

Doing that in the separate task T228857: Investigate common.js bundle content/size.

I’m still very tempted to rewrite it in plain JS and serve that as a regular old ResourceLoader module – according to find src/mediawiki/ -type f -name '*.ts' -exec cat {} + | wc -l it’s some 300 lines of TypeScript, that’s not the end of the world –

Correction: the init module also includes some files from src/data-access and a few others, so ca. 700 lines seems to be closer to the real number. (But that’s also including some interfaces and enums.)

These are not used until after loading the app module. Maybe we can also look into whether it is possible to move them there?

Good point – perhaps we can build another commonjs bundle and include that in the app ResourceLoader module.

I couldn’t figure out how to make it part of the app module, but extracting the dispatcher into a separate module reduces the size of the init module substantially:

diff --git a/client/data-bridge/package.json b/client/data-bridge/package.json
index fd81fd9f4..383145e24 100644
--- a/client/data-bridge/package.json
+++ b/client/data-bridge/package.json
@@ -8,6 +8,7 @@
     "build": "npm-run-all build:*",
     "build:app": "NODE_ENV=production vue-cli-service build --no-clean --target lib --formats commonjs src/main.ts",
     "build:init": "NODE_ENV=production vue-cli-service build --no-clean",
+    "build:dispatcher": "NODE_ENV=production vue-cli-service build --no-clean --target lib --formats commonjs --name dispatcher src/mediawiki/Dispatcher.ts",
     "fix": "vue-cli-service lint --max-warnings 0 .",
     "test": "npm-run-all test:*",
     "test:lint": "vue-cli-service lint --max-warnings 0 --no-fix . && stylelint --syntax scss 'src/**/*.(vue|scss)'",
diff --git a/client/data-bridge/src/mediawiki/init.ts b/client/data-bridge/src/mediawiki/init.ts
index 3d78cc33c..62516a32d 100644
--- a/client/data-bridge/src/mediawiki/init.ts
+++ b/client/data-bridge/src/mediawiki/init.ts
@@ -1,9 +1,10 @@
 import MwWindow from '@/@types/mediawiki/MwWindow';
 import BridgeDomElementsSelector from '@/mediawiki/BridgeDomElementsSelector';
 import { SelectedElement } from '@/mediawiki/SelectedElement';
-import Dispatcher from '@/mediawiki/Dispatcher';
+import { default as DispatcherType } from '@/mediawiki/Dispatcher';
 
 const APP_MODULE = 'wikibase.client.data-bridge.app';
+const DISPATCHER_MODULE = 'wikibase.client.data-bridge.dispatcher';
 const WBREPO_MODULE = 'mw.config.values.wbRepo';
 const FOREIGNAPI_MODULE = 'mediawiki.ForeignApi';
 const ULS_MODULE = 'jquery.uls.data';
@@ -28,12 +29,14 @@ export default async (): Promise<void> => {
 	if ( linksToOverload.length > 0 ) {
 		const require = await mwWindow.mw.loader.using( [
 				APP_MODULE,
+				DISPATCHER_MODULE,
 				WBREPO_MODULE,
 				FOREIGNAPI_MODULE,
 				ULS_MODULE,
 				MWLANGUAGE_MODULE,
 			] ),
 			app = require( APP_MODULE ),
+			Dispatcher: typeof DispatcherType = require( DISPATCHER_MODULE ).default,
 			dispatcher = new Dispatcher( mwWindow, app, dataBridgeConfig );
 
 		linksToOverload.map( ( selectedElement: SelectedElement ) => {
diff --git a/client/resources/Resources.php b/client/resources/Resources.php
index 9c9a0e0f2..085d2bccd 100644
--- a/client/resources/Resources.php
+++ b/client/resources/Resources.php
@@ -66,6 +66,24 @@
 			}
 		],
 
+		'wikibase.client.data-bridge.dispatcher' => [
+			'factory' => function () {
+				$clientSettings = WikibaseClient::getDefaultInstance()->getSettings();
+				return new ResourceLoaderFileModule(
+					[
+						'scripts' => [
+							'dispatcher.common.js'
+						],
+						'targets' => $clientSettings->getSetting( 'dataBridgeEnabled' ) ?
+							[ 'desktop', 'mobile' ] :
+							[],
+						'remoteExtPath' => 'Wikibase/client/data-bridge/dist',
+					],
+					__DIR__ . '/../data-bridge/dist'
+				);
+			},
+		],
+
 		'wikibase.client.data-bridge.app' => [
 			'factory' => function () {
 				$clientSettings = WikibaseClient::getDefaultInstance()->getSettings();
$ git show @:client/data-bridge/dist/data-bridge.init.js | wc -c
85411
$ cat client/data-bridge/dist/data-bridge.init.js | wc -c
56988

The new dispatcher.common.js is much larger than this difference, but I hope that’s mostly because it’s not minified whereas data-bridge.init.js is.

I couldn’t figure out how to make it part of the app module, but extracting the dispatcher into a separate module reduces the size of the init module substantially:

[...]
diff --git a/client/resources/Resources.php b/client/resources/Resources.php
index 9c9a0e0f2..085d2bccd 100644
--- a/client/resources/Resources.php
+++ b/client/resources/Resources.php
@@ -66,6 +66,24 @@
 			}
 		],
+		'wikibase.client.data-bridge.dispatcher' => [
+			'factory' => function () {
+				$clientSettings = WikibaseClient::getDefaultInstance()->getSettings();
+				return new ResourceLoaderFileModule(
+					[
+						'scripts' => [
+							'dispatcher.common.js'
+						],
+						'targets' => $clientSettings->getSetting( 'dataBridgeEnabled' ) ?
+							[ 'desktop', 'mobile' ] :
+							[],
+						'remoteExtPath' => 'Wikibase/client/data-bridge/dist',
+					],
+					__DIR__ . '/../data-bridge/dist'
+				);
+			},
+		],
+
 		'wikibase.client.data-bridge.app' => [
 			'factory' => function () {
 				$clientSettings = WikibaseClient::getDefaultInstance()->getSettings();

[...]

Tried it out and I think this direction is promising. But I'm not sure whether this is another of those global Resource Loader modules that are currently trying to reduce or not? Though maybe the discussion about extracting the dispatcher should happen in its own (sub-) ticket?

init.js ... Why do we send it through Babel etc. at all?
trading some development convenience for a drastic reduction in script size.

We should be able to find a configuration that works for us, instead of us working around the tool chain we picked to help us.

Can we change the order in which the initial modules are loaded?

I share your skepticism about that option.

we can start that load [of app], but then immediately add our event handlers

Agree

reduce the size of the init module

The most effective way of doing this in my eyes would be to reconsider what are "app services".
Instead of passing a ready-made service container into app we could construct it inside of it,
passing it only (based on an interface not adding compile size) what is needed to do so (e.g. a ForeignApi).
As a consequence, the concrete repo implementation would all move into app without sacrificing any
conceptual clarity. What do you think?

I’m still very tempted to rewrite it in plain JS

team discussions/decisions are a thing

dispatcher into a separate [3rd] module

By definition the sum of modules can not be smaller than before.
Rather each module carries its own copy of compatibility features and the sum is hence bigger.
When ever links are detected, both dispatcher and app would be needed - never only one. What would we gain?


Is there a majority to create tickets for the two bold statements (pun intended)?

We should be able to find a configuration that works for us, instead of us working around the tool chain we picked to help us.

I’m not proposing to work around the tool chain – I’m proposing to ditch the tool chain, in this small part of the project where I believe it doesn’t carry its weight.

dispatcher into a separate [3rd] module

By definition the sum of modules can not be smaller than before.
Rather each module carries its own copy of compatibility features and the sum is hence bigger.
When ever links are detected, both dispatcher and app would be needed - never only one. What would we gain?

Reducing the size of the sum was not the goal. The goal was to address the delayed kicking in of Bridge, i. e. to make sure it’s initialized earlier. Moving the dispatcher to a separate module and thereby reducing the size of the init module (meaning less code needs to be loaded and parsed) helps with that goal, even if it increases the total code size. That is what we would gain.

I also think that, conceptually, this is not too different from your proposal to move the services into the app module. In both cases, the important part is that we move the services out of the init module.

Is there a majority to create tickets for the two bold statements (pun intended)?

The first one seems to be completely uncontroversial, so I created a task for it already: T235767: Attach event handlers to Data Bridge links as soon as possible

edit: nevermind ^^

Created the two tickets we discussed (thanks for having the same idea!).

I suggest we close this now and take on T235765 and T235771.

On completion of the latter we should take a look at the size of the init bundle and re-evaluate if further steps are needed. I also would not be surprised if T228857: Investigate common.js bundle content/size gains us some knowledge worth applying to init as well.

Pablo-WMDE closed this task as Resolved.Oct 18 2019, 1:28 PM
Pablo-WMDE moved this task from Peer Review to Done on the Wikidata-Bridge-Sprint-7 board.
Lucas_Werkmeister_WMDE moved this task from Done to Peer Review on the Wikidata-Bridge-Sprint-7 board.

I am not yet satisfied with the (lack of) discussion about my second suggestion, to stop using webpack for the init module. (We could conceivably still use TypeScript, and add the individual resulting .js files to ResourceLoader.)

Pablo-WMDE added a comment.EditedOct 21 2019, 5:41 PM

I am not yet satisfied with the (lack of) discussion about my second suggestion, to stop using webpack for the init module

We should look at arguments, and compare pros and cons. So far we only have a suggestion for a change in technology but no measurable success criteria. Once we have them we can look into what is the solution with the least amount of downsides which gets us there. What is a file size for init you deem acceptable? How do you plan to ensure the browser matrix we claim to support stays intact long after this change is long forgotten, and for changes done by the less gifted (counting myself)?

ditch the tool chain, in this small part of the project where I believe it doesn’t carry its weight.

The problem I have with this is that the tool chain was chosen to satisfy certain demands (granted, we probably could have done a better job documenting them to be able to refer to them now), but "ditching" the tool chain without regard for features lost thereby at the faintest amount of problem does sound more like avoidance behavior rather than a solution - especially as we are talking about what is arguably the most popular JS build chain there is, not some random 3-star-github-repo. Having written this I regret time is invested into even considering that, rather than looking for a solution - but maybe it at least acts as a catalyst for us to collectively set ambitious (yet realistic) dist size goals and continuously keep an eye on them.

So far we only have a suggestion for a change in technology but no measurable success criteria.

From looking at the reports created during the building of our app, it seems realistic that we could get the init module down to 10-15 KB from currently >80 KB. Nonetheless, it would be nice to actually have a budget for this.

How do you plan to ensure the browser matrix we claim to support stays intact long after this change is long forgotten, and for changes done by the less gifted (counting myself)?

Would be nice to have cross-browser testing by default. That not being available, I guess that we would have to rely on the way it is done (or not done) for the rest of Wikibase as well.

arguably the most popular JS build chain there is, not some random 3-star-github-repo.

While that might be true, Mediawiki/Wikipedia is not a run-of-the-mill webapp where when you have seen one, you have seen them all. I'm not sure this build chain was designed to provide a build to be integrated into a preexisting extensive js environment. It would at least be nice not to include our own polyfills twice.

All these things being said, I'm not sure that the size of the init script has a big influence on the loading time (at least on anything faster than GPRS/Edge).

Ladsgroup moved this task from incoming to in progress on the Wikidata board.Oct 23 2019, 10:17 AM