resourceloader: Avoid clear/set timer overhead in mw.loader.store.add
authorTimo Tijhof <krinklemail@gmail.com>
Sat, 1 Sep 2018 03:54:47 +0000 (04:54 +0100)
committerKrinkle <krinklemail@gmail.com>
Tue, 4 Sep 2018 17:55:41 +0000 (17:55 +0000)
Follows-up d269e1d91f76 (2018), 4174b662f623b (2015).

Bug: T202598
Change-Id: I45b9f17357e6fb372456a1a4841ad948a5526437

includes/specials/SpecialJavaScriptTest.php
resources/src/startup/mediawiki.js
tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js

index 87bd2ed..c984af8 100644 (file)
@@ -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.
index 893e6d8..30f3bff 100644 (file)
                                        /**
                                         * 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.
                                         * @method
                                         */
                                        requestUpdate: ( function () {
-                                               var timer, hasPendingWrites = false;
+                                               var hasPendingWrites = false;
 
                                                function flushWrites() {
                                                        var data, key;
                                                                } );
                                                        }
 
+                                                       // 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 );
+                                                       }
                                                };
                                        }() )
                                }
index 4cdf78e..ae5ddb5 100644 (file)
@@ -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;
 
        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();
                } );
                        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++;
                } );
                                // 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
 
        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();
                } );
                        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++;
                } );
                        .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