jenkins-bot has submitted this change and it was merged.
Change subject: kvStoreMaintenance: Refactor to use requestIdleCallback
......................................................................
kvStoreMaintenance: Refactor to use requestIdleCallback
* Remove cn.doIdleWork() in favour of mw.requestIdleCallback().
This allows the browser to control when the work starts. This avoids
it from popping up at critical times related to the handling of user
interaction.
It allows better coordination with other things happening in MediaWiki
and in the browser. Its deadline defaults to 50ms at the start of each
run.
* Use the time-based deadline instead of fixed size to restrict each batch run.
This spreads out the operation dynamically based on the device's capabilities.
(E.g. "fast" devices will need less batches).
* Remove batching from getKeys(). Iteration is fast enough and we don't expect
to have more keys than we can handle in a single iteration. If for some reason
the idle time was already used up by another task, or if there are more keys
than expected, we'll fallback to ending early and processing fewer keys.
* The processKeys() function has been re-implemented to allow simple recursino
by
re-scheduling itself until its queue (in closure) is empty.
* Add unit tests.
Bug: T111456
Bug: T121646
Change-Id: Ie6d96b7f1434802adea54d07b7caeedfd2468439
---
M resources/subscribing/ext.centralNotice.bannerHistoryLogger.js
M resources/subscribing/ext.centralNotice.kvStoreMaintenance.js
M resources/subscribing/ext.centralNotice.startUp.js
A tests/qunit/subscribing/ext.centralNotice.kvStore.tests.js
4 files changed, 139 insertions(+), 125 deletions(-)
Approvals:
Ori.livneh: Looks good to me, but someone else must approve
AndyRussG: Looks good to me, approved
jenkins-bot: Verified
diff --git a/resources/subscribing/ext.centralNotice.bannerHistoryLogger.js
b/resources/subscribing/ext.centralNotice.bannerHistoryLogger.js
index aa6715a..ddba4dc 100644
--- a/resources/subscribing/ext.centralNotice.bannerHistoryLogger.js
+++ b/resources/subscribing/ext.centralNotice.bannerHistoryLogger.js
@@ -269,7 +269,7 @@
waitLogNoSendBeacon = mixinParams.waitLogNoSendBeacon;
// Do this idly to avoid browser lock-ups
- cn.doIdleWork( function() {
+ mw.requestIdleCallback( function() {
if ( !cn.kvStore.isAvailable() ) {
cn.kvStore.setNotAvailableError();
diff --git a/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js
b/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js
index e710b8c..c23268d 100644
--- a/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js
+++ b/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js
@@ -6,8 +6,8 @@
* This module provides an API at mw.centralNotice.kvStoreMaintenance.
*/
( function ( $, mw ) {
- var now = Math.round( ( new Date() ).getTime() / 1000 ),
- cn,
+ var cn,
+ now = new Date().getTime() / 1000,
// Regex to find kvStore localStorage keys. Must correspond
with PREFIX
// in ext.centralNotice.kvStore.js.
@@ -17,58 +17,67 @@
// (This should prevent race conditions among browser tabs.)
LEEWAY_FOR_REMOVAL = 86400,
- // Maximum number of keys to process at a time.
- MAX_BATCH_SIZE = 10,
+ // Minimum amount of time (in milliseconds) for an iteration
involving localStorage access.
+ MIN_WORK_TIME = 3;
- // Maximum number of items to process on any given pageview.
- // This ensures we don't block the browser too much when we
prepare the
- // arrays of keys and batch functions.
- MAX_ITEMS_TO_PROCESS = 60;
+ /**
+ * @return {jQuery.Promise} List of key strings
+ */
+ function getKeys() {
+ return $.Deferred( function ( d ) {
+ mw.requestIdleCallback( function ( deadline ) {
+ var key,
+ keys = [],
+ index = localStorage.length;
- function makeRemoveExpiredBatchFunction( keys ) {
- return function() {
-
- var n, key, rawValue, value;
-
- for ( n = 0; n < keys.length; n++ ) {
- key = keys[n];
-
- // Operate only on localStorage items used by
the kvStore
- if ( !PREFIX_REGEX.test( key ) ) {
- continue;
- }
-
- try {
- rawValue = localStorage.getItem( key );
- } catch ( e ) {
- return;
- }
-
- // The item might have been removed since we
retrieved the key
- if ( rawValue === null ) {
- continue;
- }
-
- try {
- value = JSON.parse( rawValue );
- } catch ( e ) {
- // Remove any unparseable items and
maybe set an error
- localStorage.removeItem( key );
-
- if ( cn.kvStore ) {
- cn.kvStore.setMaintenanceError(
key );
+ // We don't expect to have more keys than we
can handle in a single iteration.
+ // But just in case, ensure we don't stall for
too long.
+ while ( index-- > 0 && deadline.timeRemaining()
> MIN_WORK_TIME ) {
+ key = localStorage.key( index );
+ // Operate only on our own localStorage
items.
+ // Also recheck key existence as it may
race with other tabs.
+ if ( key !== null && PREFIX_REGEX.test(
key ) ) {
+ keys.push( key );
}
-
- continue;
}
+ d.resolve( keys );
+ } );
+ } ).promise();
+ }
- if ( !value.expiry ||
- ( value.expiry + LEEWAY_FOR_REMOVAL ) <
now ) {
-
- localStorage.removeItem( key );
+ /**
+ * @return {jQuery.Promise}
+ */
+ function processKeys( keys ) {
+ return $.Deferred( function ( d ) {
+ var queue = keys.slice();
+ mw.requestIdleCallback( function iterate( deadline ) {
+ var key, rawValue, value;
+ while ( queue[ 0 ] !== undefined &&
deadline.timeRemaining() > MIN_WORK_TIME ) {
+ key = queue.shift();
+ try {
+ rawValue =
localStorage.getItem( key );
+ if ( rawValue ) {
+ value = JSON.parse(
rawValue );
+ if ( !value.expiry || (
value.expiry + LEEWAY_FOR_REMOVAL ) < now ) {
+
localStorage.removeItem( key );
+ }
+ }
+ } catch ( e ) {
+ localStorage.removeItem( key );
+ if ( cn.kvStore ) {
+
cn.kvStore.setMaintenanceError( key );
+ }
+ }
}
- }
- };
+ if ( queue[ 0 ] !== undefined ) {
+ // Time's up, continue later
+ mw.requestIdleCallback( iterate );
+ } else {
+ d.resolve();
+ }
+ } );
+ } ).promise();
}
// Don't assume mw.centralNotice has or hasn't been initialized
@@ -80,63 +89,21 @@
cn.kvStoreMaintenance = {
/**
- * Schedule the batched removal of expired KVStore items.
+ * Start the removal of expired KVStore items.
+ *
+ * @return {jQuery.Promise}
*/
removeExpiredItemsWhenIdle: function () {
-
- var funcs, keys, i, stopBefore, key,
- j = 0,
- keysToProcess;
-
try {
- if ( !window.localStorage ||
localStorage.length === 0) {
- return;
+ if ( !window.localStorage ||
!localStorage.length ) {
+ return $.Deferred().resolve();
}
} catch ( e ) {
- return;
+ return $.Deferred().resolve();
}
- // We don't know how many batches we'll get through
before the user
- // navigates away, and there may be more localStorage
items than
- // MAX_ITEMS_TO_PROCESS. So we choose a random key
index to start
- // at, and wrap around until we've collected all keys,
or the
- // maximum number.
- // This way we're likely to get to all items
eventually, even if
- // there are a lot of them and/or each pageview is very
quick.
- i = Math.floor( Math.random() * localStorage.length );
- stopBefore =
- ( i + Math.min( MAX_ITEMS_TO_PROCESS,
localStorage.length ) )
- % localStorage.length;
-
- // Build an array of localStorage keys
- keys = [];
- do {
- key = localStorage.key( i );
-
- // Don't assume that the number of keys hasn't
changed and that
- // the key exists.
- if ( key !== null ) {
- keys.push( key );
- }
-
- i++;
- if ( i === localStorage.length ) {
- i = i - localStorage.length;
- }
- } while ( i !== stopBefore );
-
- // Build an array of functions to process the keys in
batches
- funcs = [];
- while ( j < keys.length ) {
- keysToProcess = keys.slice( j,
- Math.min( keys.length, j +
MAX_BATCH_SIZE ) );
-
- funcs.push( makeRemoveExpiredBatchFunction(
keysToProcess ) );
- j += MAX_BATCH_SIZE;
- }
-
- cn.doIdleWork( funcs );
+ return getKeys().then( processKeys );
}
};
-} )( jQuery, mediaWiki );
\ No newline at end of file
+} )( jQuery, mediaWiki );
diff --git a/resources/subscribing/ext.centralNotice.startUp.js
b/resources/subscribing/ext.centralNotice.startUp.js
index ca9f745..737bf9f 100644
--- a/resources/subscribing/ext.centralNotice.startUp.js
+++ b/resources/subscribing/ext.centralNotice.startUp.js
@@ -26,28 +26,6 @@
return;
}
- /**
- * @callback idleWorkFunc Function for performing non-time-critical
work.
- */
-
- /**
- * Temporary utility for doing non-time-critical work. See T111456.
- * Defined here 'cause it may be used by any CN subscribing modules.
- * @param {(idleWorkFunc|idleWorkFunc[])} funcs
- */
- cn.doIdleWork = function ( funcs ) {
- funcs = $.isArray( funcs ) ? funcs : [ funcs ];
-
- $( function() {
- var i;
-
- // Execute functions sequentially at intervals of 1 sec.
- for ( i = 0; i < funcs.length; i++ ) {
- setTimeout( funcs[i], 1000 * ( i + 1 ) );
- }
- } );
- };
-
// Legacy support:
// Legacy code inserted the CN div everywhere (except on Special pages),
// even when there were no campaigns. Let's do the same thing for now,
in
@@ -72,10 +50,9 @@
return;
}
- // Maintenance: clean old KV store keys whenever.
- // This schedules the removal of keys in batches. We call it via
- // doIdleWork so the scheduling itself takes place when idle, too.
- cn.doIdleWork( cn.kvStoreMaintenance.removeExpiredItemsWhenIdle );
+ // Maintenance: This schedules the removal of old KV keys.
+ // The schedule action itself is deferred, too, as it accesses
localStorage.
+ mw.requestIdleCallback(
cn.kvStoreMaintenance.removeExpiredItemsWhenIdle );
// Nothing more to do if there are no possible campaigns for this user
if ( cn.choiceData.length === 0 ) {
diff --git a/tests/qunit/subscribing/ext.centralNotice.kvStore.tests.js
b/tests/qunit/subscribing/ext.centralNotice.kvStore.tests.js
new file mode 100644
index 0000000..d27099f
--- /dev/null
+++ b/tests/qunit/subscribing/ext.centralNotice.kvStore.tests.js
@@ -0,0 +1,70 @@
+( function ( mw ) {
+ QUnit.module( 'ext.centralNotice.kvStore', QUnit.newMwEnvironment( {
+ teardown: function () {
+ var key, i = localStorage.length;
+ // Loop backwards since removal affects the key index,
+ // causing items to consistently be skipped over
+ while ( i-- > 0 ) {
+ key = localStorage.key( i );
+ if ( /^CentralNoticeKV.+\|unittest/.test( key )
) {
+ localStorage.removeItem( key );
+ }
+ }
+ }
+ } ) );
+
+ QUnit.test( 'getItem', function ( assert ) {
+ var kvStore = mw.centralNotice.kvStore,
+ context = kvStore.contexts.GLOBAL;
+ kvStore.setItem( 'unittest-New', 'x', context, 1 );
+ kvStore.setItem( 'unittest-Old', 'x', context, -2 );
+
+ assert.strictEqual( kvStore.getError(), null, 'no errors' );
+ assert.strictEqual( kvStore.getItem( 'unittest-New', context ),
'x', 'retrieve valid item' );
+ // Verify that expiry is verified at run-time regardless of
kvStoreMaintenance
+ assert.strictEqual( kvStore.getItem( 'unittest-Old', context ),
null, 'ignore expired item' );
+ } );
+
+ QUnit.test( 'maintenance', function ( assert ) {
+ var kvStore = mw.centralNotice.kvStore,
+ context = kvStore.contexts.GLOBAL,
+ done = assert.async();
+ kvStore.setItem( 'unittest-New', 'x', context, 1 );
+ kvStore.setItem( 'unittest-Old', 'x', context, -2 );
+ kvStore.setItem( 'unittest-Older', 'x', context, -3 );
+
+ assert.ok(
+ localStorage.getItem(
'CentralNoticeKV|global|unittest-New' ),
+ 'item "New" found in storage'
+ );
+ assert.ok(
+ localStorage.getItem(
'CentralNoticeKV|global|unittest-Old' ),
+ 'item "Old" found in storage'
+ );
+ assert.ok(
+ localStorage.getItem(
'CentralNoticeKV|global|unittest-Older' ),
+ 'item "Older" found in storage'
+ );
+
+
mw.centralNotice.kvStoreMaintenance.removeExpiredItemsWhenIdle().then( function
() {
+ assert.notEqual(
+ localStorage.getItem(
'CentralNoticeKV|global|unittest-New' ),
+ null,
+ 'item "New" kept in storage'
+ );
+ assert.strictEqual(
+ localStorage.getItem(
'CentralNoticeKV|global|unittest-Old' ),
+ null,
+ 'item "Old" removed from storage'
+ );
+ assert.strictEqual(
+ localStorage.getItem(
'CentralNoticeKV|global|unittest-Older' ),
+ null,
+ 'item "Older" removed from storage'
+ );
+
+ done();
+ } );
+ } );
+
+}( mediaWiki ) );
--
To view, visit https://gerrit.wikimedia.org/r/254326
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie6d96b7f1434802adea54d07b7caeedfd2468439
Gerrit-PatchSet: 17
Gerrit-Project: mediawiki/extensions/CentralNotice
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
Gerrit-Reviewer: AndyRussG <[email protected]>
Gerrit-Reviewer: Awight <[email protected]>
Gerrit-Reviewer: Cdentinger <[email protected]>
Gerrit-Reviewer: Ejegg <[email protected]>
Gerrit-Reviewer: Gilles <[email protected]>
Gerrit-Reviewer: Jforrester <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Ori.livneh <[email protected]>
Gerrit-Reviewer: Ssmith <[email protected]>
Gerrit-Reviewer: XenoRyet <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
