From 7e93521baaa4342b091a1bb7688d877af164e1e0 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Sat, 1 Sep 2018 04:54:47 +0100
Subject: [PATCH 1/1] resourceloader: Avoid clear/set timer overhead in
 mw.loader.store.add

Follows-up d269e1d91f76 (2018), 4174b662f623b (2015).

Bug: T202598
Change-Id: I45b9f17357e6fb372456a1a4841ad948a5526437
---
 includes/specials/SpecialJavaScriptTest.php   |  6 +++
 resources/src/startup/mediawiki.js            | 54 ++++++++++++-------
 .../mediawiki/mediawiki.loader.test.js        | 27 +++++-----
 3 files changed, 52 insertions(+), 35 deletions(-)

diff --git a/includes/specials/SpecialJavaScriptTest.php b/includes/specials/SpecialJavaScriptTest.php
index 87bd2ed0c9..c984af8dec 100644
--- a/includes/specials/SpecialJavaScriptTest.php
+++ b/includes/specials/SpecialJavaScriptTest.php
@@ -120,6 +120,12 @@ class SpecialJavaScriptTest extends SpecialPage {
 		$code = $rl->makeModuleResponse( $startupContext, [
 			'startup' => $rl->getModule( 'startup' ),
 		] );
+		$code .= <<<JAVASCRIPT
+	// Disable module storage.
+	// The unit test for mw.loader.store will enable it
+	// explicitly with a mock timer.
+	mw.loader.store.enabled = false;
+JAVASCRIPT;
 		// The following has to be deferred via RLQ because the startup module is asynchronous.
 		$code .= ResourceLoader::makeLoaderConditionalScript(
 			// Embed page-specific mw.config variables.
diff --git a/resources/src/startup/mediawiki.js b/resources/src/startup/mediawiki.js
index 893e6d8b8f..30f3bffa96 100644
--- a/resources/src/startup/mediawiki.js
+++ b/resources/src/startup/mediawiki.js
@@ -2256,15 +2256,27 @@
 					/**
 					 * Request a sync of the in-memory store back to persisted localStorage.
 					 *
-					 * This function debounces updates. When called with a flush already pending, the
-					 * scheduled flush is postponed. The call to localStorage.setItem will be keep
-					 * being deferred until the page is quiescent for 2 seconds.
+					 * This function debounces updates. The debouncing logic should account
+					 * for the following factors:
 					 *
-					 * Because localStorage is shared by all pages from the same origin, if multiple
-					 * pages are loaded with different module sets, the possibility exists that
-					 * modules saved by one page will be clobbered by another. The only impact of this
-					 * is minor (merely a less efficient cache use) and the problem would be corrected
-					 * by subsequent page views.
+					 * - Writing to localStorage is an expensive operation that must not happen
+					 *   during the critical path of initialising and executing module code.
+					 *   Instead, it should happen at a later time after modules have been given
+					 *   time and priority to do their thing first.
+					 *
+					 * - This method is called from mw.loader.store.add(), which will be called
+					 *   hundreds of times on a typical page, including within the same call-stack
+					 *   and eventloop-tick. This is because responses from load.php happen in
+					 *   batches. As such, we want to allow all modules from the same load.php
+					 *   response to be written to disk with a single flush, not many.
+					 *
+					 * - Repeatedly deleting and creating timers is non-trivial.
+					 *
+					 * - localStorage is shared by all pages from the same origin, if multiple
+					 *   pages are loaded with different module sets, the possibility exists that
+					 *   modules saved by one page will be clobbered by another. The impact of
+					 *   this is minor, it merely causes a less efficient cache use, and the
+					 *   problem would be corrected by subsequent page views.
 					 *
 					 * This method is considered internal to mw.loader.store and must only
 					 * be called if the store is enabled.
@@ -2273,7 +2285,7 @@
 					 * @method
 					 */
 					requestUpdate: ( function () {
-						var timer, hasPendingWrites = false;
+						var hasPendingWrites = false;
 
 						function flushWrites() {
 							var data, key;
@@ -2302,23 +2314,25 @@
 								} );
 							}
 
+							// Let the next call to requestUpdate() create a new timer.
 							hasPendingWrites = false;
 						}
 
-						function request() {
-							// If another mw.loader.store.requestUpdate() call happens in the narrow
-							// time window between requestIdleCallback() and flushWrites firing, ignore it.
-							// It'll be saved by the already-scheduled flush.
-							if ( !hasPendingWrites ) {
-								hasPendingWrites = true;
-								mw.requestIdleCallback( flushWrites );
-							}
+						function onTimeout() {
+							// Defer the actual write via requestIdleCallback
+							mw.requestIdleCallback( flushWrites );
 						}
 
 						return function () {
-							// Cancel the previous timer (if it hasn't fired yet)
-							clearTimeout( timer );
-							timer = setTimeout( request, 2000 );
+							// On the first call to requestUpdate(), create a timer that
+							// waits at least two seconds, then calls onTimeout.
+							// The main purpose is to allow the current batch of load.php
+							// responses to complete before we do anything. This batch can
+							// trigger many hundreds of calls to requestUpdate().
+							if ( !hasPendingWrites ) {
+								hasPendingWrites = true;
+								setTimeout( onTimeout, 2000 );
+							}
 						};
 					}() )
 				}
diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
index 4cdf78e541..ae5ddb587d 100644
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js
@@ -1,15 +1,12 @@
 ( function ( mw, $ ) {
 	QUnit.module( 'mediawiki.loader', QUnit.newMwEnvironment( {
 		setup: function ( assert ) {
-			mw.loader.store.enabled = false;
-
 			// Expose for load.mock.php
 			mw.loader.testFail = function ( reason ) {
 				assert.ok( false, reason );
 			};
 		},
 		teardown: function () {
-			mw.loader.store.enabled = false;
 			// Teardown for StringSet shim test
 			if ( this.nativeSet ) {
 				window.Set = this.nativeSet;
@@ -855,11 +852,8 @@
 
 	QUnit.test( 'Stale response caching - T117587', function ( assert ) {
 		var count = 0;
-		mw.loader.store.enabled = true;
-		mw.loader.register( 'test.stale', 'v2' );
-		assert.strictEqual( mw.loader.store.get( 'test.stale' ), false, 'Not in store' );
-
-		// Stub deferred timeout/idle callback
+		// Enable store and stub timeout/idle scheduling
+		this.sandbox.stub( mw.loader.store, 'enabled', true );
 		this.sandbox.stub( window, 'setTimeout', function ( fn ) {
 			fn();
 		} );
@@ -867,6 +861,9 @@
 			fn();
 		} );
 
+		mw.loader.register( 'test.stale', 'v2' );
+		assert.strictEqual( mw.loader.store.get( 'test.stale' ), false, 'Not in store' );
+
 		mw.loader.implement( 'test.stale@v1', function () {
 			count++;
 		} );
@@ -877,7 +874,7 @@
 				// After implementing, registry contains version as implemented by the response.
 				assert.strictEqual( mw.loader.getVersion( 'test.stale' ), 'v1', 'Override version' );
 				assert.strictEqual( mw.loader.getState( 'test.stale' ), 'ready' );
-				assert.ok( mw.loader.store.get( 'test.stale' ), 'In store' );
+				assert.strictEqual( typeof mw.loader.store.get( 'test.stale' ), 'string', 'In store' );
 			} )
 			.then( function () {
 				// Reset run time, but keep mw.loader.store
@@ -893,11 +890,8 @@
 
 	QUnit.test( 'Stale response caching - backcompat', function ( assert ) {
 		var script = 0;
-		mw.loader.store.enabled = true;
-		mw.loader.register( 'test.stalebc', 'v2' );
-		assert.strictEqual( mw.loader.store.get( 'test.stalebc' ), false, 'Not in store' );
-
-		// Stub deferred timeout/idle callback
+		// Enable store and stub timeout/idle scheduling
+		this.sandbox.stub( mw.loader.store, 'enabled', true );
 		this.sandbox.stub( window, 'setTimeout', function ( fn ) {
 			fn();
 		} );
@@ -905,6 +899,9 @@
 			fn();
 		} );
 
+		mw.loader.register( 'test.stalebc', 'v2' );
+		assert.strictEqual( mw.loader.store.get( 'test.stalebc' ), false, 'Not in store' );
+
 		mw.loader.implement( 'test.stalebc', function () {
 			script++;
 		} );
@@ -913,7 +910,7 @@
 			.then( function () {
 				assert.strictEqual( script, 1, 'module script ran' );
 				assert.strictEqual( mw.loader.getState( 'test.stalebc' ), 'ready' );
-				assert.ok( mw.loader.store.get( 'test.stalebc' ), 'In store' );
+				assert.strictEqual( typeof mw.loader.store.get( 'test.stalebc' ), 'string', 'In store' );
 			} )
 			.then( function () {
 				// Reset run time, but keep mw.loader.store
-- 
2.20.1