Merge pull request #14910 from Automattic/vkarpov15/gh-14897

fix(document): avoid massive perf degradation when saving new doc with 10 level deep subdocs
Automattic · Sep 25, 2024 · 34ed360 · 34ed360
2 parents 8522f39 + d273278
commit 34ed360
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 64 deletions.
diff --git a/benchmarks/createDeepNestedDocArray.js 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);
+}
diff --git 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,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;
+          }
         }
       }
     }
@@ -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];
@@ -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) {
@@ -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

diff --git 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() {