diff --git a/lib/Onyx.js b/lib/Onyx.js index 840546ca..1eaf4b06 100644 --- a/lib/Onyx.js +++ b/lib/Onyx.js @@ -4,6 +4,7 @@ import Str from 'expensify-common/lib/str'; import lodashMerge from 'lodash/merge'; import {registerLogger, logInfo, logAlert} from './Logger'; import cache from './OnyxCache'; +import createDeferredTask from './createDeferredTask'; // Keeps track of the last connectionID that was used so we can keep incrementing it let lastConnectionID = 0; @@ -28,6 +29,12 @@ const evictionBlocklist = {}; // Optional user-provided key value states set when Onyx initializes or clears let defaultKeyStates = {}; +// Cache cleaning uses this to remove least recently accessed keys +let MAX_CACHED_KEYS = 150; + +// Connections can be made before `Onyx.init`. They would wait for this promise before resolving +const deferredInit = createDeferredTask(); + /** * Get some data from the store * @@ -35,6 +42,9 @@ let defaultKeyStates = {}; * @returns {Promise<*>} */ function get(key) { + // eslint-disable-next-line no-use-before-define + addLastAccessedKey(key); + // When we already have the value in cache - resolve right away if (cache.hasCacheForKey(key)) { return Promise.resolve(cache.getValue(key)); @@ -98,19 +108,6 @@ function isCollectionKey(key) { return _.contains(_.values(onyxKeys.COLLECTION), key); } -/** - * Find the collection a collection item belongs to - * or return null if them item is not a part of a collection - * @param {string} key - * @returns {string|null} - */ -function getCollectionKeyForItem(key) { - return _.chain(onyxKeys.COLLECTION) - .values() - .find(name => key.startsWith(name)) - .value(); -} - /** * Checks to see if a given key matches with the * configured key of our connected subscriber @@ -154,7 +151,7 @@ function removeLastAccessedKey(key) { */ function addLastAccessedKey(key) { // Only specific keys belong in this list since we cannot remove an entire collection. - if (isCollectionKey(key) || !isSafeEvictionKey(key)) { + if (isCollectionKey(key)) { return; } @@ -359,18 +356,18 @@ function connect(mapping) { return connectionID; } - // Check to see if this key is flagged as a safe eviction key and add it to the recentlyAccessedKeys list - if (mapping.withOnyxInstance && !isCollectionKey(mapping.key) && isSafeEvictionKey(mapping.key)) { + // Commit connection only after init passes + deferredInit.promise.then(() => { // All React components subscribing to a key flagged as a safe eviction // key must implement the canEvict property. - if (_.isUndefined(mapping.canEvict)) { - // eslint-disable-next-line max-len - throw new Error(`Cannot subscribe to safe eviction key '${mapping.key}' without providing a canEvict value.`); + if (mapping.withOnyxInstance && !isCollectionKey(mapping.key) && isSafeEvictionKey(mapping.key)) { + if (_.isUndefined(mapping.canEvict)) { + // eslint-disable-next-line max-len + throw new Error(`Cannot subscribe to safe eviction key '${mapping.key}' without providing a canEvict value.`); + } } - addLastAccessedKey(mapping.key); - } - - getAllKeys() + }) + .then(getAllKeys) .then((keys) => { // Find all the keys matched by the config key const matchingKeys = _.filter(keys, key => isKeyMatch(mapping.key, key)); @@ -404,40 +401,28 @@ function connect(mapping) { } /** - * Remove cache items that are no longer connected through Onyx - * @param {string} key + * Used to periodically clean least recently accessed items from cache */ -function cleanCache(key) { - const hasRemainingConnections = _.some(callbackToStateMapping, {key}); - - // When the key is still used in other places don't remove it from cache - if (hasRemainingConnections) { - return; - } - - // When this is a collection - also recursively remove any unused individual items - if (isCollectionKey(key)) { - cache.drop(key); - - getAllKeys().then(cachedKeys => _.chain(cachedKeys) - .filter(name => name.startsWith(key)) - .forEach(cleanCache)); +function cleanCache() { + if (recentlyAccessedKeys.length > MAX_CACHED_KEYS) { + // Keep the most recent + const mostRecent = recentlyAccessedKeys.splice(recentlyAccessedKeys.length - MAX_CACHED_KEYS, MAX_CACHED_KEYS); + + /* Keep any other items with active connections + * Note: after `splice` the array no longer contains the `mostRecent` */ + const stillConnected = recentlyAccessedKeys.filter((key) => { + const shouldKeep = _.has(defaultKeyStates, key) + || _.some(callbackToStateMapping, mapping => key.startsWith(mapping.key)); + + if (!shouldKeep) { + cache.drop(key); + } - return; - } + return shouldKeep; + }); - // When this is a collection item - check if the collection is still used - const collectionKey = getCollectionKeyForItem(key); - if (collectionKey) { - // When there's an active subscription for a collection don't remove the item - const hasRemainingConnectionsForCollection = _.some(callbackToStateMapping, {key: collectionKey}); - if (hasRemainingConnectionsForCollection) { - return; - } + recentlyAccessedKeys = stillConnected.concat(mostRecent); } - - // Otherwise remove the value from cache - cache.drop(key); } /** @@ -457,11 +442,10 @@ function disconnect(connectionID, keyToRemoveFromEvictionBlocklist) { removeFromEvictionBlockList(keyToRemoveFromEvictionBlocklist, connectionID); } - const key = callbackToStateMapping[connectionID].key; delete callbackToStateMapping[connectionID]; - // When the last subscriber disconnects, drop cache as well - cleanCache(key); + // Check if anything can be removed from cache + cleanCache(); } /** @@ -492,7 +476,10 @@ function remove(key) { */ function evictStorageAndRetry(error, ionMethod, ...args) { // Find the first key that we can remove that has no subscribers in our blocklist - const keyForRemoval = _.find(recentlyAccessedKeys, key => !evictionBlocklist[key]); + const keyForRemoval = _.find( + recentlyAccessedKeys, + key => !evictionBlocklist[key] && isSafeEvictionKey(key), + ); if (!keyForRemoval) { logAlert('Out of storage. But found no acceptable keys to remove.'); @@ -513,6 +500,11 @@ function evictStorageAndRetry(error, ionMethod, ...args) { * @returns {Promise} */ function set(key, val) { + // Skip writing to storage if the value hasn't changed + if (cache.hasCacheForKey(key) && _.isEqual(val, cache.getValue(key))) { + return Promise.resolve(); + } + // Adds the key to cache when it's not available cache.set(key, val); @@ -607,11 +599,12 @@ function applyMerge(key, data) { function merge(key, val) { if (mergeQueue[key]) { mergeQueue[key].push(val); - return Promise.resolve(); + return merge.lastPromise; } + // Capture a promise that will resolve when this queue is written mergeQueue[key] = [val]; - return get(key) + merge.lastPromise = get(key) .then((data) => { const modifiedData = applyMerge(key, data); @@ -620,13 +613,27 @@ function merge(key, val) { delete mergeQueue[key]; return set(key, modifiedData); }); + + return merge.lastPromise; } /** - * Merge user provided default key value pairs. + * Merge stored data and user provided default key value pairs to cache + * + * @returns {Promise} */ function initializeWithDefaultKeyStates() { - _.each(defaultKeyStates, (state, key) => merge(key, state)); + return AsyncStorage.multiGet(_.keys(defaultKeyStates)) + .then((pairs) => { + const asObject = _.chain(pairs) + .map(([key, val]) => [key, val && JSON.parse(val)]) + .object() + .value(); + + const merged = lodashMerge(asObject, defaultKeyStates); + cache.merge(merged); + _.each(merged, (val, key) => keyChanged(key, val)); + }); } /** @@ -638,12 +645,13 @@ function clear() { return getAllKeys() .then((keys) => { _.each(keys, (key) => { - keyChanged(key, null); - cache.set(key, null); + if (!_.has(defaultKeyStates, key)) { + keyChanged(key, null); + cache.set(key, null); + } }); }) - .then(AsyncStorage.clear) - .then(initializeWithDefaultKeyStates); + .then(AsyncStorage.clear); } /** @@ -701,7 +709,9 @@ function mergeCollection(collectionKey, collection) { /** * Initialize the store with actions and listening for storage events * - * @param {Object} [options] + * @param {Object} options + * @param {Object} [options.keys] + * @param {Object} [options.initialKeyStates] * @param {String[]} [options.safeEvictionKeys] This is an array of keys * (individual or collection patterns) that when provided to Onyx are flagged * as "safe" for removal. Any components subscribing to these keys must also @@ -709,6 +719,8 @@ function mergeCollection(collectionKey, collection) { * @param {function} registerStorageEventListener a callback when a storage event happens. * This applies to web platforms where the local storage emits storage events * across all open tabs and allows Onyx to stay in sync across all open tabs. + * @param {Number} [options.maxCachedKeysCount=150] Sets how many recent keys should we try to keep in cache + * Setting this to 0 would only keep active connections in cache * @param {Boolean} [options.captureMetrics] */ function init({ @@ -716,6 +728,7 @@ function init({ initialKeyStates, safeEvictionKeys, registerStorageEventListener, + maxCachedKeysCount, captureMetrics = false, }) { if (captureMetrics) { @@ -724,6 +737,10 @@ function init({ applyDecorators(); } + if (_.isNumber(maxCachedKeysCount)) { + MAX_CACHED_KEYS = maxCachedKeysCount; + } + // Let Onyx know about all of our keys onyxKeys = keys; @@ -734,8 +751,9 @@ function init({ evictionAllowList = safeEvictionKeys; addAllSafeEvictionKeysToRecentlyAccessedList(); - // Initialize all of our keys with data provided - initializeWithDefaultKeyStates(); + // Initialize all of our keys with data provided then give green light to any pending connections + initializeWithDefaultKeyStates() + .finally(deferredInit.resolve); // Proceed even if the above task fails // Update any key whose value changes in storage registerStorageEventListener((key, newValue) => { @@ -775,6 +793,7 @@ function applyDecorators() { merge = decorate.decorateWithMetrics(merge, 'Onyx:merge'); mergeCollection = decorate.decorateWithMetrics(mergeCollection, 'Onyx:mergeCollection'); getAllKeys = decorate.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys'); + initializeWithDefaultKeyStates = decorate.decorateWithMetrics(initializeWithDefaultKeyStates, 'Onyx:defaults'); /* eslint-enable */ // Re-expose decorated methods diff --git a/lib/createDeferredTask.js b/lib/createDeferredTask.js new file mode 100644 index 00000000..0783a085 --- /dev/null +++ b/lib/createDeferredTask.js @@ -0,0 +1,17 @@ +/** + * Create a deferred task that can be resolved when we call `resolve()` + * The returned promise will complete only when we call `resolve` or `reject` + * Useful when we want to wait for a tasks that is resolved from an external action + * + * @template T + * @returns {{ resolve: function(*), reject: function(Error), promise: Promise }} + */ +export default function createDeferredTask() { + const deferred = {}; + deferred.promise = new Promise((res, rej) => { + deferred.resolve = res; + deferred.reject = rej; + }); + + return deferred; +} diff --git a/lib/decorateWithMetrics.js b/lib/decorateWithMetrics.js index 26f2c12d..8f42c56b 100644 --- a/lib/decorateWithMetrics.js +++ b/lib/decorateWithMetrics.js @@ -119,26 +119,45 @@ function toHumanReadableDuration(millis) { function printMetrics() { const {totalTime, averageTime, summaries} = getMetrics(); - /* eslint-disable no-console */ - console.group('Onyx Benchmark'); - console.info(' Total: ', toHumanReadableDuration(totalTime)); - console.info(' Average: ', toHumanReadableDuration(averageTime)); - - _.chain(summaries) + const prettyData = _.chain(summaries) + .filter(method => method.avg > 0) .sortBy('avg') .reverse() - .forEach(({calls, methodName, ...summary}) => { - const times = _.map(summary, (value, key) => `${key}: ${toHumanReadableDuration(value)}`); + .map(({calls, methodName, ...summary}) => { + const prettyTimes = _.chain(summary) + .map((value, key) => ([key, toHumanReadableDuration(value)])) + .object() + .value(); - console.groupCollapsed(`${methodName}\n ${times.join('\n ')} \n calls: ${calls.length}`); - console.table(calls.map(call => ({ + const prettyCalls = calls.map(call => ({ startTime: toHumanReadableDuration(call.startTime), endTime: toHumanReadableDuration(call.endTime), duration: toHumanReadableDuration(call.duration), args: JSON.stringify(call.args) - }))); - console.groupEnd(); - }); + })); + + return { + methodName, + ...prettyTimes, + calls: calls.length, + prettyCalls, + }; + }) + .value(); + + /* eslint-disable no-console */ + console.group('Onyx Benchmark'); + console.info(' Total: ', toHumanReadableDuration(totalTime)); + console.info(' Average: ', toHumanReadableDuration(averageTime)); + + console.table(prettyData.map(({prettyCalls, ...summary}) => summary)); + + prettyData.forEach((method) => { + console.groupCollapsed(`[${method.methodName}] individual calls: `); + console.table(method.prettyCalls); + console.groupEnd(); + }); + console.groupEnd(); /* eslint-enable */ } diff --git a/tests/unit/onyxCacheTest.js b/tests/unit/onyxCacheTest.js index d505265b..7ee8b205 100644 --- a/tests/unit/onyxCacheTest.js +++ b/tests/unit/onyxCacheTest.js @@ -1,5 +1,6 @@ import React from 'react'; import {render} from '@testing-library/react-native'; +import _ from 'underscore'; import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; import ViewWithText from '../components/ViewWithText'; @@ -406,7 +407,7 @@ describe('Onyx', () => { }, }; - function initOnyx() { + function initOnyx(overrides) { const OnyxModule = require('../../index'); Onyx = OnyxModule.default; withOnyx = OnyxModule.withOnyx; @@ -416,6 +417,8 @@ describe('Onyx', () => { Onyx.init({ keys: ONYX_KEYS, registerStorageEventListener: jest.fn(), + maxCachedKeysCount: 10, + ...overrides, }); // Onyx init introduces some side effects e.g. calls the getAllKeys @@ -489,6 +492,47 @@ describe('Onyx', () => { }); }); + it('Should keep recently accessed items in cache even when components unmount', () => { + // GIVEN Storage with 10 different keys + AsyncStorageMock.getItem.mockResolvedValue('"mockValue"'); + AsyncStorageMock.getAllKeys.mockResolvedValue( + _.range(10).map(number => `${ONYX_KEYS.COLLECTION.MOCK_COLLECTION}${number}`) + ); + let connections; + + // GIVEN Onyx is configured with max 5 keys in cache + return initOnyx({maxCachedKeysCount: 5}) + .then(() => { + // GIVEN 10 connections for different keys + connections = _.range(10).map((number) => { + const key = `${ONYX_KEYS.COLLECTION.MOCK_COLLECTION}${number}`; + return ({ + key, + connectionId: Onyx.connect({key}), + }); + }); + }) + .then(waitForPromisesToResolve) + .then(() => { + // WHEN they disconnect + connections.forEach(entry => Onyx.disconnect(entry.connectionId, entry.key)); + }) + .then(waitForPromisesToResolve) + .then(() => { + // THEN the most recent 5 keys should remain in cache + _.range(5, 10).forEach((number) => { + const key = connections[number].key; + expect(cache.hasCacheForKey(key)).toBe(true); + }); + + // AND the least recent 5 should be dropped + _.range(0, 5).forEach((number) => { + const key = connections[number].key; + expect(cache.hasCacheForKey(key)).toBe(false); + }); + }); + }); + it('Expect multiple calls to getItem when no existing component is using a key', () => { // GIVEN a component connected to Onyx const TestComponentWithOnyx = withOnyx({ @@ -502,7 +546,8 @@ describe('Onyx', () => { AsyncStorageMock.getAllKeys.mockResolvedValue([ONYX_KEYS.TEST_KEY]); let result; - return initOnyx() + // GIVEN Onyx that does not use LRU + return initOnyx({maxCachedKeysCount: 0}) .then(() => { // WHEN a component is rendered and unmounted and no longer available result = render(); @@ -611,7 +656,9 @@ describe('Onyx', () => { AsyncStorageMock.getAllKeys.mockResolvedValue(keys); let result; let result2; - return initOnyx() + + // GIVEN Onyx that does not use LRU + return initOnyx({maxCachedKeysCount: 0}) .then(() => { // WHEN the collection using components render result = render();