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
7 changes: 4 additions & 3 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Storage from './storage';
import * as Logger from './Logger';
import cache from './OnyxCache';
import createDeferredTask from './createDeferredTask';
import mergeWithCustomized from './mergeWithCustomized';
import fastMerge from './fastMerge';

// Keeps track of the last connectionID that was used so we can keep incrementing it
let lastConnectionID = 0;
Expand Down Expand Up @@ -761,7 +761,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)); // lodash adds a small overhead so we don't use it here
luacmartins marked this conversation as resolved.
Show resolved Hide resolved

// 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 +833,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
5 changes: 3 additions & 2 deletions lib/OnyxCache.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import _ from 'underscore';
import mergeWithCustomized from './mergeWithCustomized';
import fastMerge from './fastMerge';

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

Expand Down Expand Up @@ -110,7 +110,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)); // lodash adds a small overhead so we don't use it here
luacmartins marked this conversation as resolved.
Show resolved Hide resolved

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

let mergeObject;

/**
* Is the object Mergeable
*
* @param {*} val
* @returns {*|boolean}
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**
* Is the object Mergeable
*
* @param {*} val
* @returns {*|boolean}
*/
/**
* @param {mixed} val
* @returns {mixed}
*/

Don't include a method description unless it's valuable (and what you had there just repeats the method name). For the return value, maybe we should cast it to always return a boolean?

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]';
}

/**
* Merge Object and Arrays
*
* @param {{}|[]} target
Szymon20000 marked this conversation as resolved.
Show resolved Hide resolved
* @param {{}|[]} source
* @returns {{}|[]}
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
*/
function fastMerge(target, source) {
// eslint-disable-next-line rulesdir/prefer-underscore-method
const array = Array.isArray(source); // lodash adds a small overhead so we don't use it here
if (array) {
return source;
}
return mergeObject(target, source);
}

/**
* Merge Object
*
* @param {{}} target
* @param {{}} source
* @returns {{}}
*/
Szymon20000 marked this conversation as resolved.
Show resolved Hide resolved
mergeObject = function (target, source) {
const destination = {};
if (isMergeableObject(target)) {
// eslint-disable-next-line rulesdir/prefer-underscore-method
const targetKeys = Object.keys(target); // lodash adds a small overhead so we don't use it here
Szymon20000 marked this conversation as resolved.
Show resolved Hide resolved
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); // lodash adds a small overhead so we don't use it here
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
for (let i = 0; i < sourceKeys.length; ++i) {
const key = sourceKeys[i];
if (source[key] === undefined) {
// eslint-disable-next-line no-continue
continue;
}
if (!isMergeableObject(source[key]) || !target[key]) {
destination[key] = source[key];
} else {
destination[key] = fastMerge(target[key], source[key]);
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
}
}

return destination;
};

export default fastMerge;
26 changes: 0 additions & 26 deletions lib/mergeWithCustomized.js

This file was deleted.

5 changes: 3 additions & 2 deletions lib/storage/providers/LocalForage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import localforage from 'localforage';
import _ from 'underscore';
import SyncQueue from '../../SyncQueue';
import mergeWithCustomized from '../../mergeWithCustomized';
import fastMerge from '../../fastMerge';

localforage.config({
name: 'OnyxDB',
Expand All @@ -24,7 +24,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)) // lodash adds a small overhead so we don't use it here
Szymon20000 marked this conversation as resolved.
Show resolved Hide resolved
: value;
return localforage.setItem(key, newValue);
});
Expand Down