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
5 changes: 2 additions & 3 deletions lib/Onyx.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import Storage from './storage';
import * as Logger from './Logger';
import cache from './OnyxCache';
import createDeferredTask from './createDeferredTask';
// eslint-disable-next-line rulesdir/prefer-import-module-contents
import {fastMerge} from './fastMerge';
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 @@ -763,7 +762,7 @@ function applyMerge(key, data) {
// Object values are merged one after the other
return _.reduce(mergeValues, (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));
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
5 changes: 2 additions & 3 deletions lib/OnyxCache.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import _ from 'underscore';
// eslint-disable-next-line rulesdir/prefer-import-module-contents
import {fastMerge} from './fastMerge';
import fastMerge from './fastMerge';

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

Expand Down Expand Up @@ -112,7 +111,7 @@ class OnyxCache {
*/
merge(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));
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
56 changes: 40 additions & 16 deletions lib/fastMerge.js
Original file line number Diff line number Diff line change
@@ -1,45 +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]';
}

function mergeObject(target, source) {
/**
* 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);
const targetKeys = Object.keys(target); // lodash adds a small overhead so we don't use it here
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);
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) {
// noop
} else if (!isMergeableObject(source[key]) || !target[key]) {
// eslint-disable-next-line no-continue
continue;
}
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) {
// eslint-disable-next-line rulesdir/prefer-underscore-method
const array = Array.isArray(source);
if (array) {
return source;
}
return mergeObject(target, source);
}
export default fastMerge;
5 changes: 2 additions & 3 deletions lib/storage/providers/LocalForage.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
import localforage from 'localforage';
import _ from 'underscore';
import SyncQueue from '../../SyncQueue';
// eslint-disable-next-line rulesdir/prefer-import-module-contents
import {fastMerge} from '../../fastMerge';
import fastMerge from '../../fastMerge';

localforage.config({
name: 'OnyxDB',
Expand All @@ -26,7 +25,7 @@ const provider = {
.then((existingValue) => {
const newValue = _.isObject(existingValue)
// eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method
? Object.assign({}, fastMerge(existingValue, value))
? Object.assign({}, fastMerge(existingValue, value)) // lodash adds a small overhead so we don't use it here
: value;
return localforage.setItem(key, newValue);
});
Expand Down