From 328ddaacced5ac0ecf4fb10e054a8609d9d6c4e9 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 24 Sep 2024 15:49:14 -0400 Subject: [PATCH 1/4] fix(document): avoid massive perf degradation when saving new doc with 10 level deep subdocs Fix #14897 --- lib/document.js | 4 +++- test/document.test.js | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/document.js b/lib/document.js index 64e65df849..c12e0f03fb 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2724,7 +2724,9 @@ function _getPathsToValidate(doc, pathsToValidate, pathsToSkip) { } if (doc.$isModified(fullPathToSubdoc, null, modifiedPaths) && - !doc.isDirectModified(fullPathToSubdoc) && + // 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); diff --git a/test/document.test.js b/test/document.test.js index 6a5765fe11..7150ffe64b 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -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() { From 04263295351f32e2b0c08d3ce79c9cd36fdf98ae Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 24 Sep 2024 16:00:48 -0400 Subject: [PATCH 2/4] perf: add createDeepNestedDocArray benchmark re: #14897 --- benchmarks/createDeepNestedDocArray.js | 37 ++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 benchmarks/createDeepNestedDocArray.js diff --git a/benchmarks/createDeepNestedDocArray.js b/benchmarks/createDeepNestedDocArray.js new file mode 100644 index 0000000000..0f3ac6d4a7 --- /dev/null +++ b/benchmarks/createDeepNestedDocArray.js @@ -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); +} \ No newline at end of file From 5851261c1501fc58d3722513f708a1e87da63f4e Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 24 Sep 2024 17:30:33 -0400 Subject: [PATCH 3/4] perf(document): avoid unnecessarily pulling all subdocs when validating a subdoc --- lib/document.js | 66 ++++++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/lib/document.js b/lib/document.js index c12e0f03fb..f17e0800ff 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2689,7 +2689,7 @@ function _evaluateRequiredFunctions(doc) { * ignore */ -function _getPathsToValidate(doc, pathsToValidate, pathsToSkip) { +function _getPathsToValidate(doc, pathsToValidate, pathsToSkip, isNestedValidate) { const doValidateOptions = {}; _evaluateRequiredFunctions(doc); @@ -2709,37 +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) && - // 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.$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; + } } } } @@ -2974,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]; @@ -3061,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) { From 95500b606765147cb92751b092d69463df55d40a Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 25 Sep 2024 12:23:59 -0400 Subject: [PATCH 4/4] perf(document): remove unnecessary reset logic Re: #14897 Re: #10295 --- lib/document.js | 35 ----------------------------------- 1 file changed, 35 deletions(-) diff --git a/lib/document.js b/lib/document.js index f17e0800ff..8fe85a5a14 100644 --- a/lib/document.js +++ b/lib/document.js @@ -3484,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