Skip to content
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

[Performance] JSI Storage implementation #95

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion __mocks__/@react-native-async-storage/async-storage.js

This file was deleted.

45 changes: 23 additions & 22 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import _ from 'underscore';
import AsyncStorage from '@react-native-async-storage/async-storage';
import Str from 'expensify-common/lib/str';
import lodashMerge from 'lodash/merge';
import Storage from './storage';
Comment on lines -2 to +4
Copy link
Contributor Author

@kidroca kidroca Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using AsyncStorage or MMKV directly, we use the Storage facade

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love to see a simpler version of this PR where we do something like

import AsyncStorage from './AsyncStorage

Then have

index.js - import / export from @react-native-async-storage/async-storage
index.native.js - export a custom AsyncStorage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should use the name AsyncStorage if the underlying storage implementation could be of a different library. A more generic term should be used so it's clear we're not tied to AsyncStorage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it matters for the purposes of the POC - the code will run the same there will just be less of it.


import {registerLogger, logInfo, logAlert} from './Logger';
import cache from './OnyxCache';
import createDeferredTask from './createDeferredTask';
Expand Down Expand Up @@ -52,7 +53,7 @@ function get(key) {
}

// Otherwise retrieve the value from storage and capture a promise to aid concurrent usages
const promise = AsyncStorage.getItem(key)
const promise = Storage.getItem(key)
.then((val) => {
const parsed = val && JSON.parse(val);
cache.set(key, parsed);
Expand Down Expand Up @@ -82,7 +83,7 @@ function getAllKeys() {
}

// Otherwise retrieve the keys from storage and capture a promise to aid concurrent usages
const promise = AsyncStorage.getAllKeys()
const promise = Storage.getAllKeys()
.then((keys) => {
_.each(keys, key => cache.addKey(key));
return keys;
Expand Down Expand Up @@ -439,7 +440,7 @@ function remove(key) {
// Optimistically inform subscribers on the next tick
Promise.resolve().then(() => keyChanged(key, null));

return AsyncStorage.removeItem(key);
return Storage.removeItem(key);
}

/**
Expand Down Expand Up @@ -487,7 +488,7 @@ function set(key, val) {
Promise.resolve().then(() => keyChanged(key, val));

// Write the thing to persistent storage, which will trigger a storage event for any other tabs open on this domain
return AsyncStorage.setItem(key, JSON.stringify(val))
return Storage.setItem(key, JSON.stringify(val))
.catch(error => evictStorageAndRetry(error, set, key, val));
}

Expand Down Expand Up @@ -518,7 +519,7 @@ function multiSet(data) {
Promise.resolve().then(() => keyChanged(key, val));
});

return AsyncStorage.multiSet(keyValuePairs)
return Storage.multiSet(keyValuePairs)
.catch(error => evictStorageAndRetry(error, multiSet, data));
}

Expand Down Expand Up @@ -595,7 +596,7 @@ function merge(key, val) {
* @returns {Promise}
*/
function initializeWithDefaultKeyStates() {
return AsyncStorage.multiGet(_.keys(defaultKeyStates))
return Storage.multiGet(_.keys(defaultKeyStates))
.then((pairs) => {
const asObject = _.chain(pairs)
.map(([key, val]) => [key, val && JSON.parse(val)])
Expand All @@ -621,7 +622,7 @@ function clear() {
cache.set(key, null);
});
})
.then(AsyncStorage.clear)
.then(Storage.clear)
.then(initializeWithDefaultKeyStates);
}

Expand Down Expand Up @@ -659,11 +660,11 @@ function mergeCollection(collectionKey, collection) {
// New keys will be added via multiSet while existing keys will be updated using multiMerge
// This is because setting a key that doesn't exist yet with multiMerge will throw errors
if (keyValuePairsForExistingCollection.length > 0) {
promises.push(AsyncStorage.multiMerge(keyValuePairsForExistingCollection));
promises.push(Storage.multiMerge(keyValuePairsForExistingCollection));
}

if (keyValuePairsForNewCollection.length > 0) {
promises.push(AsyncStorage.multiSet(keyValuePairsForNewCollection));
promises.push(Storage.multiSet(keyValuePairsForNewCollection));
}

// Merge original data to cache
Expand Down Expand Up @@ -756,18 +757,18 @@ const Onyx = {
*/
function applyDecorators() {
// We're requiring the script dynamically here so that it's only evaluated when decorators are used
const decorate = require('./decorateWithMetrics');
const metering = require('./metering');

// Re-assign with decorated functions
/* eslint-disable no-func-assign */
get = decorate.decorateWithMetrics(get, 'Onyx:get');
set = decorate.decorateWithMetrics(set, 'Onyx:set');
multiSet = decorate.decorateWithMetrics(multiSet, 'Onyx:multiSet');
clear = decorate.decorateWithMetrics(clear, 'Onyx:clear');
merge = decorate.decorateWithMetrics(merge, 'Onyx:merge');
mergeCollection = decorate.decorateWithMetrics(mergeCollection, 'Onyx:mergeCollection');
getAllKeys = decorate.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys');
initializeWithDefaultKeyStates = decorate.decorateWithMetrics(initializeWithDefaultKeyStates, 'Onyx:defaults');
get = metering.decorateWithMetrics(get, 'Onyx:get');
set = metering.decorateWithMetrics(set, 'Onyx:set');
multiSet = metering.decorateWithMetrics(multiSet, 'Onyx:multiSet');
clear = metering.decorateWithMetrics(clear, 'Onyx:clear');
merge = metering.decorateWithMetrics(merge, 'Onyx:merge');
mergeCollection = metering.decorateWithMetrics(mergeCollection, 'Onyx:mergeCollection');
getAllKeys = metering.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys');
initializeWithDefaultKeyStates = metering.decorateWithMetrics(initializeWithDefaultKeyStates, 'Onyx:defaults');
/* eslint-enable */

// Re-expose decorated methods
Expand All @@ -778,9 +779,9 @@ function applyDecorators() {
Onyx.mergeCollection = mergeCollection;

// Expose stats methods on Onyx
Onyx.getMetrics = decorate.getMetrics;
Onyx.resetMetrics = decorate.resetMetrics;
Onyx.printMetrics = decorate.printMetrics;
Onyx.getMetrics = metering.getMetrics;
Onyx.resetMetrics = metering.resetMetrics;
Onyx.printMetrics = metering.printMetrics;
}

export default Onyx;
17 changes: 17 additions & 0 deletions lib/decorateWithMetrics.js → lib/metering.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,22 @@ function decorateWithMetrics(func, alias = func.name) {
return decorated;
}

function reactProfilerCallback(id, phase, actualDuration, baseDuration, startTime, commitTime, interactions) {
if (!stats[id]) {
stats[id] = [];
}

const reactStats = stats[id];
reactStats.push({
methodName: id,
startTime: startTime - PERFORMANCE_OFFSET,
duration: actualDuration,
endTime: commitTime - PERFORMANCE_OFFSET,
args: [`phase: ${phase}`, `baseDuration: ${baseDuration}`],
interactions,
});
}

/**
* Calculate the total sum of a given key in a list
* @param {Array<Record<prop, Number>>} list
Expand Down Expand Up @@ -233,4 +249,5 @@ export {
getMetrics,
resetMetrics,
printMetrics,
reactProfilerCallback,
};
3 changes: 3 additions & 0 deletions lib/storage/__mocks__/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import * as Storage from '@react-native-async-storage/async-storage/jest/async-storage-mock';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tests storage is mocked by by the AsyncStorage's mock
Currently it matches perfectly with our storage interface as the interface is based on the AsyncStorage api


export default Storage;
5 changes: 5 additions & 0 deletions lib/storage/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import StorageProvider from './providers/AsyncStorageProvider';

const instance = new StorageProvider();

export default instance;
5 changes: 5 additions & 0 deletions lib/storage/index.native.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import StorageProvider from './providers/MMKV_Provider';

const instance = new StorageProvider();

export default instance;
40 changes: 40 additions & 0 deletions lib/storage/providers/AsyncStorageProvider.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import AsyncStorage from '@react-native-async-storage/async-storage';

/**
* @implements {ProviderInterface}
*/
class AsyncStorageProvider {
async getAllKeys() {
return AsyncStorage.getAllKeys();
}

async getItem(key) {
return AsyncStorage.getItem(key);
}

async multiGet(keys) {
return AsyncStorage.multiGet(keys);
}

async removeItem(key) {
return AsyncStorage.removeItem(key);
}

async setItem(key, value) {
return AsyncStorage.setItem(key, value);
}

async multiSet(pairs) {
return AsyncStorage.multiSet(pairs);
}

async multiMerge(pairs) {
return AsyncStorage.multiMerge(pairs);
}

async clear() {
return AsyncStorage.clear();
}
}

export default AsyncStorageProvider;
63 changes: 63 additions & 0 deletions lib/storage/providers/MMKV_Provider.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import {MMKV} from 'react-native-mmkv';
import _ from 'underscore';
import lodashMerge from 'lodash/merge';

/**
* @implements {ProviderInterface}
*/
class MMKV_Provider {
async clear() {
MMKV.clearAll();
return Promise.resolve();
}

async getAllKeys() {
const keys = MMKV.getAllKeys();
return Promise.resolve(keys);
}

async getItem(key) {
const value = MMKV.getString(key);
return Promise.resolve(value);
}

async multiGet(keys) {
const pairs = keys.map(key => [key, MMKV.getString(key)]);
return Promise.resolve(pairs);
}

async multiMerge(pairs) {
pairs.forEach(([key, delta]) => {
let merged = JSON.parse(delta);
const existing = MMKV.getString(key);
if (existing) {
const parsed = JSON.parse(existing);
if (_.isObject(parsed)) {
merged = lodashMerge(parsed, delta);
}
}

const asString = JSON.stringify(merged);
MMKV.set(key, asString);
});

return Promise.resolve();
}

async multiSet(pairs) {
pairs.forEach(([key, value]) => MMKV.set(key, value));
return Promise.resolve();
}

async removeItem(key) {
MMKV.delete(key);
return Promise.resolve();
}

async setItem(key, value) {
MMKV.set(key, value);
return Promise.resolve();
}
}

export default MMKV_Provider;
75 changes: 75 additions & 0 deletions lib/storage/providers/ProviderInterface.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
function notImplemented() {
throw new Error('Method not implemented');
}

/**
* @interface
* The interface Onyx uses to communicate with the storage layer
*/
class ProviderInterface {
Comment on lines +5 to +9
Copy link
Contributor Author

@kidroca kidroca Aug 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can serve as an abstract class and even provide some base functionality like parsing/stringifying data

ATM no-one is extending or using the class created here it serves for documentation purposes

  • declaring a new Provider and annotating it with @implements {ProviderInterface} would prompt to implement all methods (Webstorm™)
  • Providers will also inherit all method documentation (and param types)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-one is extending or using the class created here it serves for documentation purposes

Can we maybe explore this when there is a clear need? It feels like an unnecessary distraction for the current exercise.

/**
* @returns {Promise<string[]>}
*/
async getAllKeys() {
return notImplemented();
}

/**
* @template T
* @param {string} key
* @returns {Promise<T>}
*/
async getItem(key) {
return notImplemented(key);
}

/**
* @param {string[]} keys
* @returns {Promise<Array<[string, *]>>}
*/
async multiGet(keys) {
return notImplemented(keys);
}

/**
* @param {string} key
* @returns {Promise<void>}
*/
async removeItem(key) {
notImplemented(key);
}

/**
* @param {string} key
* @param {*} value
* @returns {Promise<void>}
*/
async setItem(key, value) {
notImplemented(key, value);
}

/**
* @param {Array<[string, *]>} pairs
* @returns {Promise<void>}
*/
async multiSet(pairs) {
notImplemented(pairs);
}

/**
* @param {Array<[string, *]>} pairs
* @returns {Promise<void>}
*/
async multiMerge(pairs) {
notImplemented(pairs);
}

/**
* @returns {Promise<void>}
*/
async clear() {
notImplemented();
}
}

export default ProviderInterface;
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
},
"peerDependencies": {
"@react-native-async-storage/async-storage": "^1.15.5",
"react": "^17.0.2"
"react": "^17.0.2",
"react-native-mmkv": "^1.2.2"
},
"jest": {
"preset": "react-native",
Expand All @@ -69,7 +70,8 @@
"timers": "fake",
"testEnvironment": "jsdom",
"setupFilesAfterEnv": [
"@testing-library/jest-native/extend-expect"
"@testing-library/jest-native/extend-expect",
"<rootDir>/tests/setupAfterEnv.js"
]
}
}
1 change: 1 addition & 0 deletions tests/setupAfterEnv.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
jest.mock('../lib/storage');
Loading