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.

Sign up for GitHub

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

Jump to bottom

perf(document): avoid creating unnecessary empty objects when creating a state machine #11988

Merged
merged 7 commits into from Jul 18, 2022
Merged

perf(document): avoid creating unnecessary empty objects when creating a state machine #11988

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4a2e65e
perf(document): avoid creating unnecessary empty objects when creatin…
vkarpov15 Jun 25, 2022
1ccbed6
fix: missed a couple of spots in #11988
vkarpov15 Jun 25, 2022
989525c
perf(document+statemachine): remove some unnecessary duplicated fields
vkarpov15 Jun 26, 2022
9e05baf
perf(document+internal): put default value for `$isNew` and `strictMo…
vkarpov15 Jun 26, 2022
3a1c821
perf(schema): correctly pull DocumentArrayPath default `_id` value wh…
vkarpov15 Jun 26, 2022
8f4cc45
Merge branch '6.5' into vkarpov15/gh-11541
vkarpov15 Jul 18, 2022
fa29f3a
perf(connection): move unnecessary check out of for loop
vkarpov15 Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/connection.js
View file
Open in desktop
Expand Up @@ -493,6 +493,9 @@ Connection.prototype.transaction = function transaction(fn, options) {

for (const path of state.modifiedPaths) {
doc.$__.activePaths.paths[path] = 'modify';
if (doc.$__.activePaths.states.modify == null) {
doc.$__.activePaths.states.modify = {};
}
Copy link
Collaborator

@hasezoey hasezoey Jun 27, 2022 •
edited

Choose a reason for hiding this comment

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

if i understand this check correctly, then this could be done out of the loop (for (const path of state.modifiedPaths)) because it does not need to be repeated, only a condition would need to be added to check that state.modifiedPaths is not empty as to not unnecessary set it when empty

i did a small test with performance across 3 different points with 3 runs each:

// first i ran on branch 6.5, where
memavg = 147.31509908040366
timeavg = 1944.6666666666667

// then i ran on branch vkarpov15/gh-11541, where
memavg = 120.52366638183594
timeavg = 1887.3333333333333

// then i modified and moved the if outside of the mentioned loop, where
memavg = 121.06150309244792
timeavg = 1834.3333333333333
Run Numbers 
run1mem=[145.24718475341797,151.04356384277344,145.65454864501953]
run1time=[1938,1952,1944]
run2mem=[119.71839141845703,120.27562713623047,121.57698059082031]
run2time=[1929,1861,1872]
run3mem=[120.03515625,122.43016052246094,120.71919250488281]
run3time=[1812,1849,1842]

sum = (arr) => {let sum=0;for(const a of arr) {sum+=a} return sum;}
avg = (arr) => {let asum = sum(arr); return (asum / arr.length) || 0;}

avg(run1mem);
avg(run1time);
avg(run2mem);
avg(run2time);
avg(run3mem);
avg(run3time);

Note: the if i used is:

if (state.modifiedPaths.length > 0 && doc.$__.activePaths.states.modify == null) {
  doc.$__.activePaths.states.modify = {};
}

and i had also modified the test script a bit: https://gist.github.com/hasezoey/6bc112e760b05e9a059550aa8e3dbbfa

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion 👍

doc.$__.activePaths.states.modify[path] = true;
}

Expand Down
68 changes: 39 additions & 29 deletions lib/document.js
View file
Open in desktop
Expand Up @@ -93,7 +93,11 @@ function Document(obj, fields, skipId, options) {
}

this.$__ = new InternalCache();
this.$isNew = 'isNew' in options ? options.isNew : true;

// Avoid setting `isNew` to `true`, because it is `true` by default
if (options.isNew != null && options.isNew !== true) {
this.$isNew = options.isNew;
}

if (options.priorDoc != null) {
this.$__.priorDoc = options.priorDoc;
Expand All @@ -116,13 +120,12 @@ function Document(obj, fields, skipId, options) {
const schema = this.$__schema;

if (typeof fields === 'boolean' || fields === 'throw') {
this.$__.strictMode = fields;
if (fields !== true) {
this.$__.strictMode = fields;
}
fields = undefined;
} else {
} else if (schema.options.strict !== true) {
this.$__.strictMode = schema.options.strict;
if (fields != null) {
this.$__.selected = fields;
}
}

const requiredPaths = schema.requiredPaths(true);
Expand All @@ -134,9 +137,9 @@ function Document(obj, fields, skipId, options) {

// determine if this doc is a result of a query with
// excluded fields
if (utils.isPOJO(fields)) {
if (utils.isPOJO(fields) && Object.keys(fields).length > 0) {
exclude = isExclusive(fields);
this.$__.fields = fields;
this.$__.selected = fields;
this.$__.exclude = exclude;
}

Expand Down Expand Up @@ -210,6 +213,13 @@ Object.defineProperty(Document.prototype, 'errors', {
this.$errors = value;
}
});

/*!
* ignore
*/

Document.prototype.$isNew = true;

/*!
* Document exposes the NodeJS event emitter API, so you can use
* `on`, `once`, etc.
Expand Down Expand Up @@ -745,10 +755,10 @@ Document.prototype.$__init = function(doc, opts) {
this.$emit('init', this);
this.constructor.emit('init', this);

const hasIncludedChildren = this.$__.exclude === false && this.$__.fields ?
$__hasIncludedChildren(this.$__.fields) :
const hasIncludedChildren = this.$__.exclude === false && this.$__.selected ?
$__hasIncludedChildren(this.$__.selected) :
null;
$__applyDefaults(this, this.$__.fields, this.$__.exclude, hasIncludedChildren, false, this.$__.skipDefaults);
$__applyDefaults(this, this.$__.selected, this.$__.exclude, hasIncludedChildren, false, this.$__.skipDefaults);

return this;
};
Expand Down Expand Up @@ -1588,7 +1598,7 @@ Document.prototype.$__shouldModify = function(pathToMark, path, options, constru
return true;
}

if (val === void 0 && path in this.$__.activePaths.states.default) {
if (val === void 0 && path in this.$__.activePaths.getStatePaths('default')) {
// we're just unsetting the default value which was never saved
return false;
}
Expand All @@ -1608,7 +1618,7 @@ Document.prototype.$__shouldModify = function(pathToMark, path, options, constru
if (!constructing &&
val !== null &&
val !== undefined &&
path in this.$__.activePaths.states.default &&
path in this.$__.activePaths.getStatePaths('default') &&
deepEqual(val, schema.getDefault(this, constructing))) {
// a path with a default was $unset on the server
// and the user is setting it to the same value again
Expand Down Expand Up @@ -1906,7 +1916,7 @@ Document.prototype.$ignore = function(path) {
*/

Document.prototype.directModifiedPaths = function() {
return Object.keys(this.$__.activePaths.states.modify);
return Object.keys(this.$__.activePaths.getStatePaths('modify'));
};

/**
Expand Down Expand Up @@ -1984,7 +1994,7 @@ function _isEmpty(v) {
Document.prototype.modifiedPaths = function(options) {
options = options || {};

const directModifiedPaths = Object.keys(this.$__.activePaths.states.modify);
const directModifiedPaths = Object.keys(this.$__.activePaths.getStatePaths('modify'));
const result = new Set();

let i = 0;
Expand Down Expand Up @@ -2063,7 +2073,7 @@ Document.prototype[documentModifiedPaths] = Document.prototype.modifiedPaths;

Document.prototype.isModified = function(paths, modifiedPaths) {
if (paths) {
const directModifiedPaths = Object.keys(this.$__.activePaths.states.modify);
const directModifiedPaths = Object.keys(this.$__.activePaths.getStatePaths('modify'));
if (directModifiedPaths.length === 0) {
return false;
}
Expand Down Expand Up @@ -2113,15 +2123,15 @@ Document.prototype.$isDefault = function(path) {
}

if (typeof path === 'string' && path.indexOf(' ') === -1) {
return this.$__.activePaths.states.default.hasOwnProperty(path);
return this.$__.activePaths.getStatePaths('default').hasOwnProperty(path);
}

let paths = path;
if (!Array.isArray(paths)) {
paths = paths.split(' ');
}

return paths.some(path => this.$__.activePaths.states.default.hasOwnProperty(path));
return paths.some(path => this.$__.activePaths.getStatePaths('default').hasOwnProperty(path));
};

/**
Expand Down Expand Up @@ -2174,15 +2184,15 @@ Document.prototype.isDirectModified = function(path) {
}

if (typeof path === 'string' && path.indexOf(' ') === -1) {
return this.$__.activePaths.states.modify.hasOwnProperty(path);
return this.$__.activePaths.getStatePaths('modify').hasOwnProperty(path);
}

let paths = path;
if (!Array.isArray(paths)) {
paths = paths.split(' ');
}

return paths.some(path => this.$__.activePaths.states.modify.hasOwnProperty(path));
return paths.some(path => this.$__.activePaths.getStatePaths('modify').hasOwnProperty(path));
};

/**
Expand All @@ -2199,15 +2209,15 @@ Document.prototype.isInit = function(path) {
}

if (typeof path === 'string' && path.indexOf(' ') === -1) {
return this.$__.activePaths.states.init.hasOwnProperty(path);
return this.$__.activePaths.getStatePaths('init').hasOwnProperty(path);
}

let paths = path;
if (!Array.isArray(paths)) {
paths = paths.split(' ');
}

return paths.some(path => this.$__.activePaths.states.init.hasOwnProperty(path));
return paths.some(path => this.$__.activePaths.getStatePaths('init').hasOwnProperty(path));
};

/**
Expand Down Expand Up @@ -2435,7 +2445,7 @@ Document.prototype.$validate = Document.prototype.validate;
*/

function _evaluateRequiredFunctions(doc) {
const requiredFields = Object.keys(doc.$__.activePaths.states.require);
const requiredFields = Object.keys(doc.$__.activePaths.getStatePaths('require'));
let i = 0;
const len = requiredFields.length;
for (i = 0; i < len; ++i) {
Expand Down Expand Up @@ -2463,7 +2473,7 @@ function _getPathsToValidate(doc) {

_evaluateRequiredFunctions(doc);
// only validate required fields when necessary
let paths = new Set(Object.keys(doc.$__.activePaths.states.require).filter(function(path) {
let paths = new Set(Object.keys(doc.$__.activePaths.getStatePaths('require')).filter(function(path) {
if (!doc.$__isSelected(path) && !doc.$isModified(path)) {
return false;
}
Expand All @@ -2473,9 +2483,9 @@ function _getPathsToValidate(doc) {
return true;
}));

Object.keys(doc.$__.activePaths.states.init).forEach(addToPaths);
Object.keys(doc.$__.activePaths.states.modify).forEach(addToPaths);
Object.keys(doc.$__.activePaths.states.default).forEach(addToPaths);
Object.keys(doc.$__.activePaths.getStatePaths('init')).forEach(addToPaths);
Object.keys(doc.$__.activePaths.getStatePaths('modify')).forEach(addToPaths);
Object.keys(doc.$__.activePaths.getStatePaths('default')).forEach(addToPaths);
function addToPaths(p) { paths.add(p); }

const subdocs = doc.$getAllSubdocs();
Expand Down Expand Up @@ -3197,8 +3207,8 @@ Document.prototype.$__reset = function reset() {

this.$__.backup = {};
this.$__.backup.activePaths = {
modify: Object.assign({}, this.$__.activePaths.states.modify),
default: Object.assign({}, this.$__.activePaths.states.default)
modify: Object.assign({}, this.$__.activePaths.getStatePaths('modify')),
default: Object.assign({}, this.$__.activePaths.getStatePaths('default'))
};
this.$__.backup.validationError = this.$__.validationError;
this.$__.backup.errors = this.$errors;
Expand Down
6 changes: 3 additions & 3 deletions lib/helpers/document/cleanModifiedSubpaths.js
View file
Open in desktop
Expand Up @@ -13,21 +13,21 @@ module.exports = function cleanModifiedSubpaths(doc, path, options) {
return deleted;
}

for (const modifiedPath of Object.keys(doc.$__.activePaths.states.modify)) {
for (const modifiedPath of Object.keys(doc.$__.activePaths.getStatePaths('modify'))) {
if (skipDocArrays) {
const schemaType = doc.$__schema.path(modifiedPath);
if (schemaType && schemaType.$isMongooseDocumentArray) {
continue;
}
}
if (modifiedPath.startsWith(path + '.')) {
delete doc.$__.activePaths.states.modify[modifiedPath];
doc.$__.activePaths.clearPath(modifiedPath);
++deleted;

if (doc.$isSubdocument) {
const owner = doc.ownerDocument();
const fullPath = doc.$__fullPath(modifiedPath);
delete owner.$__.activePaths.states.modify[fullPath];
owner.$__.activePaths.clearPath(fullPath);
}
}
}
Expand Down
13 changes: 7 additions & 6 deletions lib/helpers/document/compile.js
View file
Open in desktop
Expand Up @@ -17,6 +17,13 @@ const isPOJO = utils.isPOJO;
exports.compile = compile;
exports.defineKey = defineKey;

const _isEmptyOptions = Object.freeze({
minimize: true,
virtuals: false,
getters: false,
transform: false
});

/*!
* Compiles schemas.
*/
Expand Down Expand Up @@ -130,12 +137,6 @@ function defineKey({ prop, subprops, prototype, prefix, options }) {
value: true
});

const _isEmptyOptions = Object.freeze({
minimize: true,
virtuals: false,
getters: false,
transform: false
});
Object.defineProperty(nested, '$isEmpty', {
enumerable: false,
configurable: true,
Expand Down
3 changes: 2 additions & 1 deletion lib/internal.js
View file
Open in desktop
Expand Up @@ -13,8 +13,9 @@ function InternalCache() {
this.activePaths = new ActiveRoster();
}

InternalCache.prototype.strictMode = true;

InternalCache.prototype.fullPath = undefined;
InternalCache.prototype.strictMode = undefined;
InternalCache.prototype.selected = undefined;
InternalCache.prototype.shardval = undefined;
InternalCache.prototype.saveError = undefined;
Expand Down
4 changes: 2 additions & 2 deletions lib/plugins/trackTransaction.js
View file
Open in desktop
Expand Up @@ -23,14 +23,14 @@ module.exports = function trackTransaction(schema) {
initialState.versionKey = this.get(this.$__schema.options.versionKey);
}

initialState.modifiedPaths = new Set(Object.keys(this.$__.activePaths.states.modify));
initialState.modifiedPaths = new Set(Object.keys(this.$__.activePaths.getStatePaths('modify')));
initialState.atomics = _getAtomics(this);

session[sessionNewDocuments].set(this, initialState);
} else {
const state = session[sessionNewDocuments].get(this);

for (const path of Object.keys(this.$__.activePaths.states.modify)) {
for (const path of Object.keys(this.$__.activePaths.getStatePaths('modify'))) {
state.modifiedPaths.add(path);
}
state.atomics = _getAtomics(this, state.atomics);
Expand Down
7 changes: 7 additions & 0 deletions lib/schema/documentarray.js
View file
Open in desktop
Expand Up @@ -36,6 +36,13 @@ let Subdocument;
*/

function DocumentArrayPath(key, schema, options, schemaOptions) {
const schemaTypeIdOption = DocumentArrayPath.defaultOptions &&
DocumentArrayPath.defaultOptions._id;
if (schemaTypeIdOption != null) {
schemaOptions = schemaOptions || {};
schemaOptions._id = schemaTypeIdOption;
}

if (schemaOptions != null && schemaOptions._id != null) {
schema = handleIdOption(schema, schemaOptions);
} else if (options != null && options._id != null) {
Expand Down
32 changes: 23 additions & 9 deletions lib/statemachine.js
View file
Open in desktop
Expand Up @@ -38,19 +38,12 @@ StateMachine.ctor = function() {
StateMachine.apply(this, arguments);
this.paths = {};
this.states = {};
this.stateNames = states;

let i = states.length,
state;

while (i--) {
state = states[i];
this.states[state] = {};
}
};

ctor.prototype = new StateMachine();

ctor.prototype.stateNames = states;

states.forEach(function(state) {
// Changes the `path`'s state to `state`.
ctor.prototype[state] = function(path) {
Expand All @@ -76,6 +69,7 @@ StateMachine.prototype._changeState = function _changeState(path, nextState) {
if (prevBucket) delete prevBucket[path];

this.paths[path] = nextState;
this.states[nextState] = this.states[nextState] || {};
this.states[nextState][path] = true;
};

Expand All @@ -84,6 +78,9 @@ StateMachine.prototype._changeState = function _changeState(path, nextState) {
*/

StateMachine.prototype.clear = function clear(state) {
if (this.states[state] == null) {
return;
}
const keys = Object.keys(this.states[state]);
let i = keys.length;
let path;
Expand All @@ -108,6 +105,17 @@ StateMachine.prototype.clearPath = function clearPath(path) {
delete this.states[state][path];
};

/*!
* Gets the paths for the given state, or empty object `{}` if none.
*/

StateMachine.prototype.getStatePaths = function getStatePaths(state) {
if (this.states[state] != null) {
return this.states[state];
}
return {};
};

/*!
* Checks to see if at least one path is in the states passed in via `arguments`
* e.g., this.some('required', 'inited')
Expand All @@ -120,6 +128,9 @@ StateMachine.prototype.some = function some() {
const _this = this;
const what = arguments.length ? arguments : this.stateNames;
return Array.prototype.some.call(what, function(state) {
if (_this.states[state] == null) {
return false;
}
return Object.keys(_this.states[state]).length;
});
};
Expand All @@ -143,6 +154,9 @@ StateMachine.prototype._iter = function _iter(iterMethod) {
const _this = this;

const paths = states.reduce(function(paths, state) {
if (_this.states[state] == null) {
return paths;
}
return paths.concat(Object.keys(_this.states[state]));
}, []);

Expand Down