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

fix(document): avoid massive perf degradation when saving new doc with 10 level deep subdocs #14910

Merged
merged 5 commits into from
Sep 25, 2024
Merged
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
37 changes: 37 additions & 0 deletions benchmarks/createDeepNestedDocArray.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';

const mongoose = require('../');

run().catch(err => {
console.error(err);
process.exit(-1);
});

async function run() {
await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_benchmark');

const levels = 12;

let schema = new mongoose.Schema({ test: { type: String, required: true } });
let doc = { test: 'gh-14897' };
for (let i = 0; i < levels; ++i) {
schema = new mongoose.Schema({ level: Number, subdocs: [schema] });
doc = { level: (levels - i), subdocs: [{ ...doc }, { ...doc }] };
}
const Test = mongoose.model('Test', schema);

if (!process.env.MONGOOSE_BENCHMARK_SKIP_SETUP) {
await Test.deleteMany({});
}

const insertStart = Date.now();
await Test.create(doc);
const insertEnd = Date.now();

const results = {
'create() time ms': +(insertEnd - insertStart).toFixed(2)
};

console.log(JSON.stringify(results, null, ' '));
process.exit(0);
}
99 changes: 35 additions & 64 deletions lib/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -2689,7 +2689,7 @@ function _evaluateRequiredFunctions(doc) {
* ignore
*/

function _getPathsToValidate(doc, pathsToValidate, pathsToSkip) {
function _getPathsToValidate(doc, pathsToValidate, pathsToSkip, isNestedValidate) {
const doValidateOptions = {};

_evaluateRequiredFunctions(doc);
Expand All @@ -2709,35 +2709,40 @@ function _getPathsToValidate(doc, pathsToValidate, pathsToSkip) {
Object.keys(doc.$__.activePaths.getStatePaths('default')).forEach(addToPaths);
function addToPaths(p) { paths.add(p); }

const subdocs = doc.$getAllSubdocs();
const modifiedPaths = doc.modifiedPaths();
for (const subdoc of subdocs) {
if (subdoc.$basePath) {
const fullPathToSubdoc = subdoc.$isSingleNested ? subdoc.$__pathRelativeToParent() : subdoc.$__fullPathWithIndexes();

// Remove child paths for now, because we'll be validating the whole
// subdoc.
// The following is a faster take on looping through every path in `paths`
// and checking if the path starts with `fullPathToSubdoc` re: gh-13191
for (const modifiedPath of subdoc.modifiedPaths()) {
paths.delete(fullPathToSubdoc + '.' + modifiedPath);
}
if (!isNestedValidate) {
// If we're validating a subdocument, all this logic will run anyway on the top-level document, so skip for subdocuments
const subdocs = doc.$getAllSubdocs();
const modifiedPaths = doc.modifiedPaths();
for (const subdoc of subdocs) {
if (subdoc.$basePath) {
const fullPathToSubdoc = subdoc.$isSingleNested ? subdoc.$__pathRelativeToParent() : subdoc.$__fullPathWithIndexes();

// Remove child paths for now, because we'll be validating the whole
// subdoc.
// The following is a faster take on looping through every path in `paths`
// and checking if the path starts with `fullPathToSubdoc` re: gh-13191
for (const modifiedPath of subdoc.modifiedPaths()) {
paths.delete(fullPathToSubdoc + '.' + modifiedPath);
}

if (doc.$isModified(fullPathToSubdoc, null, modifiedPaths) &&
!doc.isDirectModified(fullPathToSubdoc) &&
!doc.$isDefault(fullPathToSubdoc)) {
paths.add(fullPathToSubdoc);
if (doc.$isModified(fullPathToSubdoc, null, modifiedPaths) &&
// Avoid using isDirectModified() here because that does additional checks on whether the parent path
// is direct modified, which can cause performance issues re: gh-14897
!doc.$__.activePaths.getStatePaths('modify').hasOwnProperty(fullPathToSubdoc) &&
!doc.$isDefault(fullPathToSubdoc)) {
paths.add(fullPathToSubdoc);

if (doc.$__.pathsToScopes == null) {
doc.$__.pathsToScopes = {};
}
doc.$__.pathsToScopes[fullPathToSubdoc] = subdoc.$isDocumentArrayElement ?
subdoc.__parentArray :
subdoc.$parent();
if (doc.$__.pathsToScopes == null) {
doc.$__.pathsToScopes = {};
}
doc.$__.pathsToScopes[fullPathToSubdoc] = subdoc.$isDocumentArrayElement ?
subdoc.__parentArray :
subdoc.$parent();

doValidateOptions[fullPathToSubdoc] = { skipSchemaValidators: true };
if (subdoc.$isDocumentArrayElement && subdoc.__index != null) {
doValidateOptions[fullPathToSubdoc].index = subdoc.__index;
doValidateOptions[fullPathToSubdoc] = { skipSchemaValidators: true };
if (subdoc.$isDocumentArrayElement && subdoc.__index != null) {
doValidateOptions[fullPathToSubdoc].index = subdoc.__index;
}
}
}
}
Expand Down Expand Up @@ -2972,7 +2977,7 @@ Document.prototype.$__validate = function(pathsToValidate, options, callback) {
paths = [...paths];
doValidateOptionsByPath = {};
} else {
const pathDetails = _getPathsToValidate(this, pathsToValidate, pathsToSkip);
const pathDetails = _getPathsToValidate(this, pathsToValidate, pathsToSkip, options && options._nestedValidate);
paths = shouldValidateModifiedOnly ?
pathDetails[0].filter((path) => this.$isModified(path)) :
pathDetails[0];
Expand Down Expand Up @@ -3059,7 +3064,8 @@ Document.prototype.$__validate = function(pathsToValidate, options, callback) {
const doValidateOptions = {
...doValidateOptionsByPath[path],
path: path,
validateAllPaths
validateAllPaths,
_nestedValidate: true
};

schemaType.doValidate(val, function(err) {
Expand Down Expand Up @@ -3478,44 +3484,9 @@ Document.prototype.$__reset = function reset() {
// Skip for subdocuments
const subdocs = !this.$isSubdocument ? this.$getAllSubdocs() : null;
if (subdocs && subdocs.length > 0) {
const resetArrays = new Set();
for (const subdoc of subdocs) {
const fullPathWithIndexes = subdoc.$__fullPathWithIndexes();
subdoc.$__reset();
if (this.isModified(fullPathWithIndexes) || isParentInit(fullPathWithIndexes)) {
if (subdoc.$isDocumentArrayElement) {
resetArrays.add(subdoc.parentArray());
} else {
const parent = subdoc.$parent();
if (parent === this) {
this.$__.activePaths.clearPath(subdoc.$basePath);
} else if (parent != null && parent.$isSubdocument) {
// If map path underneath subdocument, may end up with a case where
// map path is modified but parent still needs to be reset. See gh-10295
parent.$__reset();
}
}
}
}

for (const array of resetArrays) {
this.$__.activePaths.clearPath(array.$path());
array[arrayAtomicsBackupSymbol] = array[arrayAtomicsSymbol];
array[arrayAtomicsSymbol] = {};
}
}

function isParentInit(path) {
path = path.indexOf('.') === -1 ? [path] : path.split('.');
let cur = '';
for (let i = 0; i < path.length; ++i) {
cur += (cur.length ? '.' : '') + path[i];
if (_this.$__.activePaths[cur] === 'init') {
return true;
}
}

return false;
}

// clear atomics
Expand Down
21 changes: 21 additions & 0 deletions test/document.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13905,6 +13905,27 @@ describe('document', function() {
const objectWithGetters = result.toObject({ getters: true, virtuals: false });
assert.strictEqual(objectWithGetters.level1.level2.level3.property, 'TESTVALUE');
});

it('handles inserting and saving large document with 10-level deep subdocs (gh-14897)', async function() {
const levels = 10;

let schema = new Schema({ test: { type: String, required: true } });
let doc = { test: 'gh-14897' };
for (let i = 0; i < levels; ++i) {
schema = new Schema({ level: Number, subdocs: [schema] });
doc = { level: (levels - i), subdocs: [{ ...doc }, { ...doc }] };
}

const Test = db.model('Test', schema);
const savedDoc = await Test.create(doc);

let cur = savedDoc;
for (let i = 0; i < levels - 1; ++i) {
cur = cur.subdocs[0];
}
cur.subdocs[0] = { test: 'updated' };
await savedDoc.save();
});
});

describe('Check if instance function that is supplied in schema option is available', function() {
Expand Down