-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Onyx optimizations #88
Closed
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a06856e
Updated Onyx.printMetrics functionality - easier copy to excel
kidroca 85f2356
fix: some connections are triggered before `Onyx.init` is executed
kidroca f29cfc9
feat: implement `multiGet` function
kidroca cc0d468
refactor: extract a `createDeferredTask` helper
kidroca 137012b
feat: use `multiGet` in connect calls
kidroca c5a4ba6
perf: Skip a write to storage when nothing is actually changing
kidroca 49aa17c
perf: Better cache strategy. Allow non safe eviction keys in recently…
kidroca 60fa625
Revert multiGet changes, rendering can't cope
kidroca 11b6b8b
refactor: better merge promise handling
kidroca 3ae5fa8
perf: merge defaultKeyStates in one go
kidroca d6e05e7
perf: Don't write defaults to hard storage but just in cache
kidroca 93c30be
perf: Don't remove default keys from cache
kidroca File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,13 +29,22 @@ 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 | ||
* | ||
* @param {string} key | ||
* @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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check |
||
); | ||
|
||
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<void>} | ||
*/ | ||
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,21 +709,26 @@ 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 | ||
* implement a canEvict option. See the README for more info. | ||
* @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({ | ||
keys, | ||
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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<T|void> }} | ||
*/ | ||
export default function createDeferredTask() { | ||
const deferred = {}; | ||
deferred.promise = new Promise((res, rej) => { | ||
deferred.resolve = res; | ||
deferred.reject = rej; | ||
}); | ||
|
||
return deferred; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was updated to allow to keep more items in the access list
To preserve the same functionality the check was moved here: Line 481