Skip to content

Commit

Permalink
Merge pull request #554 from margelo/@chrispader/remove-null-values-f…
Browse files Browse the repository at this point in the history 
…rom-cache

Remove null values from cache to improve Onyx read speed
  • Loading branch information
@mountiny
mountiny authored May 30, 2024
2 parents 81b1f4b + 11deea5 commit 31858f1
Show file tree
Hide file tree
Showing 12 changed files with 289 additions and 173 deletions.
98 changes: 71 additions & 27 deletions lib/Onyx.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function init({

if (shouldSyncMultipleInstances) {
Storage.keepInstancesSync?.((key, value) => {
const prevValue = cache.getValue(key, false) as OnyxValue<typeof key>;
const prevValue = cache.get(key, false) as OnyxValue<typeof key>;
cache.set(key, value);
OnyxUtils.keyChanged(key, value as OnyxValue<typeof key>, prevValue);
});
Expand Down Expand Up @@ -111,7 +111,7 @@ function connect<TKey extends OnyxKey>(connectOptions: ConnectOptions<TKey>): nu
// Performance improvement
// If the mapping is connected to an onyx key that is not a collection
// we can skip the call to getAllKeys() and return an array with a single item
if (Boolean(mapping.key) && typeof mapping.key === 'string' && !mapping.key.endsWith('_') && cache.storageKeys.has(mapping.key)) {
if (Boolean(mapping.key) && typeof mapping.key === 'string' && !mapping.key.endsWith('_') && cache.getAllKeys().has(mapping.key)) {
return new Set([mapping.key]);
}
return OnyxUtils.getAllKeys();
Expand All @@ -128,12 +128,12 @@ function connect<TKey extends OnyxKey>(connectOptions: ConnectOptions<TKey>): nu
// component. This null value will be filtered out so that the connected component can utilize defaultProps.
if (matchingKeys.length === 0) {
if (mapping.key && !OnyxUtils.isCollectionKey(mapping.key)) {
cache.set(mapping.key, null);
cache.addNullishStorageKey(mapping.key);
}

// Here we cannot use batching because the null value is expected to be set immediately for default props
// Here we cannot use batching because the nullish value is expected to be set immediately for default props
// or they will be undefined.
OnyxUtils.sendDataToConnection(mapping, null as OnyxValue<TKey>, undefined, false);
OnyxUtils.sendDataToConnection(mapping, null, undefined, false);
return;
}

Expand Down Expand Up @@ -210,8 +210,20 @@ function disconnect(connectionID: number, keyToRemoveFromEvictionBlocklist?: Ony
* @param value value to store
*/
function set<TKey extends OnyxKey>(key: TKey, value: NonUndefined<OnyxEntry<KeyValueMapping[TKey]>>): Promise<void> {
// check if the value is compatible with the existing value in the storage
const existingValue = cache.getValue(key, false);
// When we use Onyx.set to set a key we want to clear the current delta changes from Onyx.merge that were queued
// before the value was set. If Onyx.merge is currently reading the old value from storage, it will then not apply the changes.
if (OnyxUtils.hasPendingMergeForKey(key)) {
delete OnyxUtils.getMergeQueue()[key];
}

const existingValue = cache.get(key, false);

// If the existing value as well as the new value are null, we can return early.
if (value === null && existingValue === null) {
return Promise.resolve();
}

// Check if the value is compatible with the existing value in the storage
const {isCompatible, existingValueType, newValueType} = utils.checkCompatibilityWithExistingValue(value, existingValue);
if (!isCompatible) {
Logger.logAlert(logMessages.incompatibleUpdateAlert(key, 'set', existingValueType, newValueType));
Expand All @@ -220,22 +232,29 @@ function set<TKey extends OnyxKey>(key: TKey, value: NonUndefined<OnyxEntry<KeyV

// If the value is null, we remove the key from storage
const {value: valueAfterRemoving, wasRemoved} = OnyxUtils.removeNullValues(key, value);
const valueWithoutNullValues = valueAfterRemoving as OnyxValue<TKey>;

if (OnyxUtils.hasPendingMergeForKey(key)) {
delete OnyxUtils.getMergeQueue()[key];
const logSetCall = (hasChanged = true) => {
// Logging properties only since values could be sensitive things we don't want to log
Logger.logInfo(`set called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''} hasChanged: ${hasChanged}`);
};

// Calling "OnyxUtils.removeNullValues" removes the key from storage and cache and updates the subscriber.
// Therefore, we don't need to further broadcast and update the value so we can return early.
if (wasRemoved) {
logSetCall();
return Promise.resolve();
}

const valueWithoutNullValues = valueAfterRemoving as OnyxValue<TKey>;
const hasChanged = cache.hasValueChanged(key, valueWithoutNullValues);

// Logging properties only since values could be sensitive things we don't want to log
Logger.logInfo(`set called for key: ${key}${_.isObject(value) ? ` properties: ${_.keys(value).join(',')}` : ''} hasChanged: ${hasChanged}`);
logSetCall(hasChanged);

// This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNullValues, hasChanged, wasRemoved);
const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNullValues, hasChanged);

// If the value has not changed or the key got removed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
if (!hasChanged || wasRemoved) {
if (!hasChanged) {
return updatePromise;
}

Expand All @@ -255,21 +274,33 @@ function set<TKey extends OnyxKey>(key: TKey, value: NonUndefined<OnyxEntry<KeyV
* @param data object keyed by ONYXKEYS and the values to set
*/
function multiSet(data: Partial<NullableKeyValueMapping>): Promise<void> {
const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data, true);
const allKeyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(data, true);

// When a key is set to null, we need to remove the remove the key from storage using "OnyxUtils.remove".
// Therefore, we filter the key value pairs to exclude null values and remove those keys explicitly.
const removePromises: Array<Promise<void>> = [];
const keyValuePairsToUpdate = allKeyValuePairs.filter(([key, value]) => {
if (value === null) {
removePromises.push(OnyxUtils.remove(key));
return false;
}

return true;
});

const updatePromises = keyValuePairs.map(([key, value]) => {
const prevValue = cache.getValue(key, false);
const updatePromises = keyValuePairsToUpdate.map(([key, value]) => {
const prevValue = cache.get(key, false);

// Update cache and optimistically inform subscribers on the next tick
cache.set(key, value);
return OnyxUtils.scheduleSubscriberUpdate(key, value, prevValue);
});

return Storage.multiSet(keyValuePairs)
return Storage.multiSet(allKeyValuePairs)
.catch((error) => OnyxUtils.evictStorageAndRetry(error, multiSet, data))
.then(() => {
OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, data);
return Promise.all(updatePromises);
return Promise.all([removePromises, updatePromises]);
})
.then(() => undefined);
}
Expand Down Expand Up @@ -311,7 +342,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: NonUndefined<OnyxEntry<
mergeQueuePromise[key] = OnyxUtils.get(key).then((existingValue) => {
// Calls to Onyx.set after a merge will terminate the current merge process and clear the merge queue
if (mergeQueue[key] == null) {
return undefined;
return Promise.resolve();
}

try {
Expand All @@ -326,7 +357,7 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: NonUndefined<OnyxEntry<
});

if (!validChanges.length) {
return undefined;
return Promise.resolve();
}
const batchedDeltaChanges = OnyxUtils.applyMerge(undefined, validChanges, false);

Expand All @@ -339,9 +370,21 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: NonUndefined<OnyxEntry<
delete mergeQueue[key];
delete mergeQueuePromise[key];

const logMergeCall = (hasChanged = true) => {
// Logging properties only since values could be sensitive things we don't want to log
Logger.logInfo(`merge called for key: ${key}${_.isObject(batchedDeltaChanges) ? ` properties: ${_.keys(batchedDeltaChanges).join(',')}` : ''} hasChanged: ${hasChanged}`);
};

// If the batched changes equal null, we want to remove the key from storage, to reduce storage size
const {wasRemoved} = OnyxUtils.removeNullValues(key, batchedDeltaChanges);

// Calling "OnyxUtils.removeNullValues" removes the key from storage and cache and updates the subscriber.
// Therefore, we don't need to further broadcast and update the value so we can return early.
if (wasRemoved) {
logMergeCall();
return Promise.resolve();
}

// For providers that can't handle delta changes, we need to merge the batched changes with the existing value beforehand.
// The "preMergedValue" will be directly "set" in storage instead of being merged
// Therefore we merge the batched changes with the existing value to get the final merged value that will be stored.
Expand All @@ -351,14 +394,13 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: NonUndefined<OnyxEntry<
// In cache, we don't want to remove the key if it's null to improve performance and speed up the next merge.
const hasChanged = cache.hasValueChanged(key, preMergedValue);

// Logging properties only since values could be sensitive things we don't want to log
Logger.logInfo(`merge called for key: ${key}${_.isObject(batchedDeltaChanges) ? ` properties: ${_.keys(batchedDeltaChanges).join(',')}` : ''} hasChanged: ${hasChanged}`);
logMergeCall(hasChanged);

// This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
const updatePromise = OnyxUtils.broadcastUpdate(key, preMergedValue as OnyxValue<TKey>, hasChanged, wasRemoved);
const updatePromise = OnyxUtils.broadcastUpdate(key, preMergedValue as OnyxValue<TKey>, hasChanged);

// If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead.
if (!hasChanged || wasRemoved) {
if (!hasChanged) {
return updatePromise;
}

Expand Down Expand Up @@ -518,6 +560,8 @@ function mergeCollection<TKey extends CollectionKeyBase, TMap>(collectionKey: TK
function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
return OnyxUtils.getAllKeys()
.then((keys) => {
cache.clearNullishStorageKeys();

const keysToBeClearedFromStorage: OnyxKey[] = [];
const keyValuesToResetAsCollection: Record<OnyxKey, OnyxCollection<KeyValueMapping[OnyxKey]>> = {};
const keyValuesToResetIndividually: NullableKeyValueMapping = {};
Expand All @@ -537,7 +581,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {
// 2. Figure out whether it is a collection key or not,
// since collection key subscribers need to be updated differently
if (!isKeyToPreserve) {
const oldValue = cache.getValue(key);
const oldValue = cache.get(key);
const newValue = defaultKeyStates[key] ?? null;
if (newValue !== oldValue) {
cache.set(key, newValue);
Expand Down Expand Up @@ -565,7 +609,7 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise<void> {

// Notify the subscribers for each key/value group so they can receive the new values
Object.entries(keyValuesToResetIndividually).forEach(([key, value]) => {
updatePromises.push(OnyxUtils.scheduleSubscriberUpdate(key, value, cache.getValue(key, false)));
updatePromises.push(OnyxUtils.scheduleSubscriberUpdate(key, value, cache.get(key, false)));
});
Object.entries(keyValuesToResetAsCollection).forEach(([key, value]) => {
updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value));
Expand Down
98 changes: 65 additions & 33 deletions lib/OnyxCache.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ import type {OnyxKey, OnyxValue} from './types';
*/
class OnyxCache {
/** Cache of all the storage keys available in persistent storage */
storageKeys: Set<OnyxKey>;
private storageKeys: Set<OnyxKey>;

/** A list of keys where a nullish value has been fetched from storage before, but the key still exists in cache */
private nullishStorageKeys: Set<OnyxKey>;

/** Unique list of keys maintained in access order (most recent at the end) */
private recentKeys: Set<OnyxKey>;
Expand All @@ -28,6 +31,7 @@ class OnyxCache {

constructor() {
this.storageKeys = new Set();
this.nullishStorageKeys = new Set();
this.recentKeys = new Set();
this.storageMap = {};
this.pendingPromises = new Map();
Expand All @@ -36,9 +40,11 @@ class OnyxCache {
bindAll(
this,
'getAllKeys',
'getValue',
'get',
'hasCacheForKey',
'addKey',
'addNullishStorageKey',
'clearNullishStorageKeys',
'set',
'drop',
'merge',
Expand All @@ -57,19 +63,18 @@ class OnyxCache {
}

/**
* Get a cached value from storage
* @param [shouldReindexCache] – This is an LRU cache, and by default accessing a value will make it become last in line to be evicted. This flag can be used to skip that and just access the value directly without side-effects.
* Allows to set all the keys at once.
* This is useful when we are getting
* all the keys from the storage provider
* and we want to keep the cache in sync.
*
* Previously, we had to call `addKey` in a loop
* to achieve the same result.
*
* @param keys - an array of keys
*/
getValue(key: OnyxKey, shouldReindexCache = true): OnyxValue<OnyxKey> {
if (shouldReindexCache) {
this.addToAccessedKeys(key);
}
return this.storageMap[key];
}

/** Check whether cache has data for the given key */
hasCacheForKey(key: OnyxKey): boolean {
return this.storageMap[key] !== undefined;
setAllKeys(keys: OnyxKey[]) {
this.storageKeys = new Set(keys);
}

/** Saves a key in the storage keys list
Expand All @@ -79,13 +84,49 @@ class OnyxCache {
this.storageKeys.add(key);
}

/** Used to set keys that are null/undefined in storage without adding null to the storage map */
addNullishStorageKey(key: OnyxKey): void {
this.nullishStorageKeys.add(key);
}

/** Used to clear keys that are null/undefined in cache */
clearNullishStorageKeys(): void {
this.nullishStorageKeys = new Set();
}

/** Check whether cache has data for the given key */
hasCacheForKey(key: OnyxKey): boolean {
return this.storageMap[key] !== undefined || this.nullishStorageKeys.has(key);
}

/**
* Get a cached value from storage
* @param [shouldReindexCache] – This is an LRU cache, and by default accessing a value will make it become last in line to be evicted. This flag can be used to skip that and just access the value directly without side-effects.
*/
get(key: OnyxKey, shouldReindexCache = true): OnyxValue<OnyxKey> {
if (shouldReindexCache) {
this.addToAccessedKeys(key);
}
return this.storageMap[key];
}

/**
* Set's a key value in cache
* Adds the key to the storage keys list as well
*/
set(key: OnyxKey, value: OnyxValue<OnyxKey>): OnyxValue<OnyxKey> {
this.addKey(key);
this.addToAccessedKeys(key);

// When a key is explicitly set in cache, we can remove it from the list of nullish keys,
// since it will either be set to a non nullish value or removed from the cache completely.
this.nullishStorageKeys.delete(key);

if (value === null || value === undefined) {
delete this.storageMap[key];
return undefined;
}

this.storageMap[key] = value;

return value;
Expand All @@ -107,27 +148,18 @@ class OnyxCache {
throw new Error('data passed to cache.merge() must be an Object of onyx key/value pairs');
}

this.storageMap = {...utils.fastMerge(this.storageMap, data, false)};
this.storageMap = {...utils.fastMerge(this.storageMap, data)};

const storageKeys = this.getAllKeys();
const mergedKeys = Object.keys(data);
this.storageKeys = new Set([...storageKeys, ...mergedKeys]);
mergedKeys.forEach((key) => this.addToAccessedKeys(key));
}
Object.entries(data).forEach(([key, value]) => {
this.addKey(key);
this.addToAccessedKeys(key);

/**
* Allows to set all the keys at once.
* This is useful when we are getting
* all the keys from the storage provider
* and we want to keep the cache in sync.
*
* Previously, we had to call `addKey` in a loop
* to achieve the same result.
*
* @param keys - an array of keys
*/
setAllKeys(keys: OnyxKey[]) {
this.storageKeys = new Set(keys);
if (value === null || value === undefined) {
this.addNullishStorageKey(key);
} else {
this.nullishStorageKeys.delete(key);
}
});
}

/**
Expand Down
Loading

0 comments on commit 31858f1

Please sign in to comment.