Krinkle has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/340185 )
Change subject: kvStore: Minor clean up and optimization of kvStoreMaintenance ...................................................................... kvStore: Minor clean up and optimization of kvStoreMaintenance * Reduce memory by working on the given array of keys directly instead of copying it first. Due to lexical scope, the original had to be retained too. * Minor whitespace clean up for consistency. * A few notes based inspiration from <https://github.com/pamelafox/lscache/> that will help make this more robust for T121646. Bug: T121646 Change-Id: I3dbcb0a43e955d07117489c864056ab8f493ba95 --- M resources/subscribing/ext.centralNotice.kvStore.js M resources/subscribing/ext.centralNotice.kvStoreMaintenance.js M resources/subscribing/ext.centralNotice.startUp.js 3 files changed, 14 insertions(+), 18 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice refs/changes/85/340185/1 diff --git a/resources/subscribing/ext.centralNotice.kvStore.js b/resources/subscribing/ext.centralNotice.kvStore.js index e635c51..673ac48 100644 --- a/resources/subscribing/ext.centralNotice.kvStore.js +++ b/resources/subscribing/ext.centralNotice.kvStore.js @@ -53,7 +53,6 @@ * TODO Should this go in core? */ function areCookiesEnabled() { - // On the first call, set a cookie and try to read it back if ( cookiesEnabled === null ) { @@ -73,7 +72,6 @@ * compatibility and certain user privacy options are required.) */ function isLocalStorageAvailable() { - if ( localStorageAvailable === null ) { // For the KV store to work, the browser has to support @@ -108,7 +106,6 @@ * @param {KVStorageContext} context */ function setError( message, key, value, context ) { - error = { message: message, key: key, @@ -138,7 +135,6 @@ * @return {string} */ function makeKeyForLocalStorage( key, context ) { - var base = PREFIX + SEPARATOR + context.key + SEPARATOR; switch ( context.key ) { @@ -169,7 +165,6 @@ * @return {string} */ function makeKeyForCookie( key, context ) { - var base = PREFIX_IN_COOKIES + SEPARATOR_IN_COOKIES + context.keyInCookies + SEPARATOR_IN_COOKIES; @@ -190,7 +185,6 @@ } function setLocalStorageItem( key, value, context, ttl ) { - var lsKey, encodedWrappedValue; lsKey = makeKeyForLocalStorage( key, context ); @@ -222,7 +216,6 @@ } function setCookieItem( key, value, context, ttl ) { - return Boolean( $.cookie( makeKeyForCookie( key, context ), encodeURIComponent( JSON.stringify( value ) ), @@ -231,7 +224,6 @@ } function getLocalStorageItem( key, context ) { - var lsKey = makeKeyForLocalStorage( key, context ), rawValue, wrappedValue; @@ -253,6 +245,9 @@ wrappedValue = JSON.parse( rawValue ); } catch ( e ) { + // FIXME: Consider detecting out-of-space errors and perform + // garbage-collection immediately, starting by removing the oldest + // expired (and unexpired) keys older than a certain threshold. // If the JSON couldn't be parsed, log and return null (which is // the same value we'd get if the key were not set). @@ -287,7 +282,6 @@ } function getCookieItem( key, context ) { - var storageKey = makeKeyForCookie( key, context ), rawCookie = $.cookie( storageKey ); @@ -301,7 +295,6 @@ } function removeLocalStorageItem( key, context ) { - try { localStorage.removeItem( makeKeyForLocalStorage( key, context ) ); @@ -377,7 +370,6 @@ * @return {boolean} true if the value could be set, false otherwise */ setItem: function ( key, value, context, ttl, multiStorageOption ) { - // Check validity of key if ( ( key.indexOf( SEPARATOR ) !== -1 ) || ( key.indexOf( SEPARATOR_IN_COOKIES ) !== -1 ) ) { @@ -417,7 +409,6 @@ * Defaults to kvStore.multiStorageOptions.LOCAL_STORAGE. */ getItem: function ( key, context, multiStorageOption ) { - multiStorageOption = multiStorageOption || kvStore.multiStorageOptions.LOCAL_STORAGE; @@ -450,7 +441,6 @@ * Defaults to kvStore.multiStorageOptions.LOCAL_STORAGE. */ removeItem: function ( key, context, multiStorageOption ) { - multiStorageOption = multiStorageOption || kvStore.multiStorageOptions.LOCAL_STORAGE; @@ -488,7 +478,6 @@ * @return {string} A string key */ getMultiStorageOption: function ( cookieAllowed ) { - if ( isLocalStorageAvailable() ) { return kvStore.multiStorageOptions.LOCAL_STORAGE; } @@ -526,11 +515,11 @@ campaignName = cName; }, - setBannerName: function ( bName ) { + setBannerName: function ( bName ) { bannerName = bName; }, - setCategory: function ( c ) { + setCategory: function ( c ) { category = c; } }; diff --git a/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js b/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js index 77a9df6..7cb9a86 100644 --- a/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js +++ b/resources/subscribing/ext.centralNotice.kvStoreMaintenance.js @@ -52,9 +52,8 @@ /** * @return {jQuery.Promise} */ - function processKeys( keys ) { + function processKeys( queue ) { 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 ) { diff --git a/resources/subscribing/ext.centralNotice.startUp.js b/resources/subscribing/ext.centralNotice.startUp.js index b22fa87..3d8fed5 100644 --- a/resources/subscribing/ext.centralNotice.startUp.js +++ b/resources/subscribing/ext.centralNotice.startUp.js @@ -53,6 +53,14 @@ // Maintenance: This schedules the removal of old KV keys. // The schedule action itself is deferred, too, as it accesses localStorage. + // - FIXME: Consider doing this behind a random sample instead of every page view + // (e.g. 50% or 20%). Or, instead of sampling: + // - FIXME: Use sessionStorage (mw.storage.session) to not schedule multiple + // maintenance windows simultaneously and to not schedule maintenance too + // often (e.g. no more once every 6 hours). It may be attractive to do + // "No more than once per session" but a time-bound is still needed as + // sessions often never end due to save-on-quit and restore-on-reopen features + // in many products. mw.requestIdleCallback( cn.kvStoreMaintenance.doMaintenance ); // Nothing more to do if there are no possible campaigns for this user -- To view, visit https://gerrit.wikimedia.org/r/340185 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3dbcb0a43e955d07117489c864056ab8f493ba95 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/CentralNotice Gerrit-Branch: master Gerrit-Owner: Krinkle <krinklem...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
