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

Use faster implementation for merging #186

Merged
merged 16 commits into from
Oct 5, 2022
8 changes: 5 additions & 3 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import Storage from './storage';
import * as Logger from './Logger';
import cache from './OnyxCache';
import createDeferredTask from './createDeferredTask';
import mergeWithCustomized from './mergeWithCustomized';
// eslint-disable-next-line rulesdir/prefer-import-module-contents
import {fastMerge} from './fastMerge';
luacmartins marked this conversation as resolved.
Show resolved Hide resolved

// Keeps track of the last connectionID that was used so we can keep incrementing it
let lastConnectionID = 0;
Expand Down Expand Up @@ -761,7 +762,8 @@ function applyMerge(key, data) {
if (_.isObject(data) || _.every(mergeValues, _.isObject)) {
// Object values are merged one after the other
return _.reduce(mergeValues, (modifiedData, mergeValue) => {
const newData = mergeWithCustomized({}, modifiedData, mergeValue);
// eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
const newData = Object.assign({}, fastMerge(modifiedData, mergeValue));

// We will also delete any object keys that are undefined or null.
// Deleting keys is not supported by AsyncStorage so we do it this way.
Expand Down Expand Up @@ -832,7 +834,7 @@ function initializeWithDefaultKeyStates() {
.then((pairs) => {
const asObject = _.object(pairs);

const merged = mergeWithCustomized(asObject, defaultKeyStates);
const merged = fastMerge(asObject, defaultKeyStates);
cache.merge(merged);
_.each(merged, (val, key) => keyChanged(key, val));
});
Expand Down
6 changes: 4 additions & 2 deletions lib/OnyxCache.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import _ from 'underscore';
import mergeWithCustomized from './mergeWithCustomized';
// eslint-disable-next-line rulesdir/prefer-import-module-contents
import {fastMerge} from './fastMerge';

const isDefined = _.negate(_.isUndefined);

Expand Down Expand Up @@ -110,7 +111,8 @@ class OnyxCache {
* @param {Record<string, *>} data - a map of (cache) key - values
*/
merge(data) {
this.storageMap = mergeWithCustomized({}, this.storageMap, data);
// eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
this.storageMap = Object.assign({}, fastMerge(this.storageMap, data));

const storageKeys = this.getAllKeys();
const mergedKeys = _.keys(data);
Expand Down
43 changes: 43 additions & 0 deletions lib/fastMerge.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Mostly copied from https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1

function isMergeableObject(val) {
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
const nonNullObject = val && typeof val === 'object';
return nonNullObject
&& Object.prototype.toString.call(val) !== '[object RegExp]'
&& Object.prototype.toString.call(val) !== '[object Date]';
}

function mergeObject(target, source) {
const destination = {};
if (isMergeableObject(target)) {
// eslint-disable-next-line rulesdir/prefer-underscore-method
const targetKeys = Object.keys(target);
for (let i = 0; i < targetKeys.length; ++i) {
const key = targetKeys[i];
destination[key] = target[key];
}
}
// eslint-disable-next-line rulesdir/prefer-underscore-method
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason not to use underscore here? If it's because of performance, please add a code comment explaining that.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the function has to be defined as mergeObject = function (target, source) instead of simply function mergeObject(target, source)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have to do that otherwise we would use it before it's defined, since we have the following circular dependency mergeObject -> fastMerge -> mergeObject

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

when 2 functions are on the same level, they are registered at the same time, it's just that eslint would flag it as a code style issue, because our code style is to define stuff before it's used - impossible to address in case of a circular dependency, but we can just disable the warning

The actual problem is with variables - you can't use a variable before it is defined, but you can use a function before it's defined, because functions are always registered before any code runs

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @kidroca. In that case, I agree that we should declare it as function mergeObject() and disable the lint warning with a comment explaining why

const sourceKeys = Object.keys(source);
for (let i = 0; i < sourceKeys.length; ++i) {
const key = sourceKeys[i];
if (!isMergeableObject(source[key]) || !target[key]) {
destination[key] = source[key];
} else {
// eslint-disable-next-line no-use-before-define
destination[key] = fastMerge(target[key], source[key]);
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
}
}

return destination;
}

// eslint-disable-next-line import/prefer-default-export, rulesdir/no-inline-named-export
export function fastMerge(target, source) {
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line rulesdir/prefer-underscore-method
const array = Array.isArray(source);
if (array) {
return source;
}
return mergeObject(target, source);
}
26 changes: 0 additions & 26 deletions lib/mergeWithCustomized.js

This file was deleted.

6 changes: 4 additions & 2 deletions lib/storage/providers/LocalForage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
import localforage from 'localforage';
import _ from 'underscore';
import SyncQueue from '../../SyncQueue';
import mergeWithCustomized from '../../mergeWithCustomized';
// eslint-disable-next-line rulesdir/prefer-import-module-contents
import {fastMerge} from '../../fastMerge';

localforage.config({
name: 'OnyxDB',
Expand All @@ -24,7 +25,8 @@ const provider = {
return localforage.getItem(key)
.then((existingValue) => {
const newValue = _.isObject(existingValue)
? mergeWithCustomized({}, existingValue, value)
// eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method
? Object.assign({}, fastMerge(existingValue, value))
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
: value;
return localforage.setItem(key, newValue);
});
Expand Down