Merge pull request #13280 from Automattic/vkarpov15/gh-13191

perf: speed up creating maps of subdocuments
Automattic · Apr 27, 2023 · e364017 · e364017
2 parents c9f5b19 + b9ca7ca
commit e364017
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 16 deletions.
diff --git a/benchmarks/mapOfSubdocs.js b/benchmarks/mapOfSubdocs.js
@@ -0,0 +1,55 @@
+'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_test', {
+    serverSelectionTimeoutMS: 5000
+  });
+
+  const minisMap = new mongoose.Schema(
+    {
+      //? Mini reference
+      mini: {
+        type: Map,
+        of: new mongoose.Schema({
+          //? Mini ID
+          miniID: { type: mongoose.Schema.Types.ObjectId, ref: 'Mini', required: true },
+          //? Timestamp as number
+          timestamp: { type: Number, required: true },
+        }),
+      },
+    },
+    //? Automatic creation of timestamps for creation and updating.
+    //? This will be created on the background by the package
+    { timestamps: true }
+  );
+  const MinisMap = mongoose.model('MinisMap', minisMap);
+  await MinisMap.init();
+
+  const mini = new Map();
+  for (let i = 0; i < 2000; ++i) {
+    const miniID = new mongoose.Types.ObjectId();
+    mini.set(miniID, {
+      miniID,
+      timestamp: Math.floor(Math.random() * 1000000)
+    });
+  }
+
+  let loopStart = Date.now();
+
+  for (let k = 0; k < 10; k++) {
+    await MinisMap.create({ mini });
+  }
+
+  const results = {
+    'Average save time ms': (Date.now() - loopStart) / 10
+  };
+
+  console.log(JSON.stringify(results, null, '  '));
+}
diff --git a/lib/document.js b/lib/document.js
@@ -2221,19 +2221,26 @@ Document.prototype[documentModifiedPaths] = Document.prototype.modifiedPaths;
 
 Document.prototype.isModified = function(paths, modifiedPaths) {
   if (paths) {
-    const directModifiedPaths = Object.keys(this.$__.activePaths.getStatePaths('modify'));
-    if (directModifiedPaths.length === 0) {
-      return false;
-    }
-
     if (!Array.isArray(paths)) {
       paths = paths.indexOf(' ') === -1 ? [paths] : paths.split(' ');
     }
+
+    const directModifiedPathsObj = this.$__.activePaths.states.modify;
+    if (directModifiedPathsObj == null) {
+      return false;
+    }
+    for (const path of paths) {
+      if (Object.prototype.hasOwnProperty.call(directModifiedPathsObj, path)) {
+        return true;
+      }
+    }
+
     const modified = modifiedPaths || this[documentModifiedPaths]();
     const isModifiedChild = paths.some(function(path) {
       return !!~modified.indexOf(path);
     });
 
+    const directModifiedPaths = Object.keys(directModifiedPathsObj);
     return isModifiedChild || paths.some(function(path) {
       return directModifiedPaths.some(function(mod) {
         return mod === path || path.startsWith(mod + '.');

diff --git a/lib/types/map.js b/lib/types/map.js
@@ -96,9 +96,10 @@ class MongooseMap extends Map {
       return;
     }
 
-    const fullPath = this.$__path + '.' + key;
-    const populated = this.$__parent != null && this.$__parent.$__ ?
-      this.$__parent.$populated(fullPath, true) || this.$__parent.$populated(this.$__path, true) :
+    let _fullPath;
+    const parent = this.$__parent;
+    const populated = parent != null && parent.$__ && parent.$__.populated ?
+      parent.$populated(fullPath.call(this), true) || parent.$populated(this.$__path, true) :
       null;
     const priorVal = this.get(key);
 
@@ -127,11 +128,19 @@ class MongooseMap extends Map {
       }
     } else {
       try {
-        value = this.$__schemaType.
-          applySetters(value, this.$__parent, false, this.get(key), { path: fullPath });
+        const options = this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested ?
+          { path: fullPath.call(this) } :
+          null;
+        value = this.$__schemaType.applySetters(
+          value,
+          this.$__parent,
+          false,
+          this.get(key),
+          options
+        );
       } catch (error) {
         if (this.$__parent != null && this.$__parent.$__ != null) {
-          this.$__parent.invalidate(fullPath, error);
+          this.$__parent.invalidate(fullPath.call(this), error);
           return;
         }
         throw error;
@@ -140,13 +149,18 @@ class MongooseMap extends Map {
 
     super.set(key, value);
 
-    if (value != null && value.$isSingleNested) {
-      value.$basePath = this.$__path + '.' + key;
+    if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) {
+      parent.markModified(fullPath.call(this));
     }
 
-    const parent = this.$__parent;
-    if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) {
-      parent.markModified(this.$__path + '.' + key);
+    // Delay calculating full path unless absolutely necessary, because string
+    // concatenation is a bottleneck re: #13171
+    function fullPath() {
+      if (_fullPath) {
+        return _fullPath;
+      }
+      _fullPath = this.$__path + '.' + key;
+      return _fullPath;
     }
   }
 

diff --git a/test/types.map.test.js b/test/types.map.test.js
@@ -956,6 +956,7 @@ describe('Map', function() {
     });
     doc.myMap.set('abc', { myValue: 'some value' });
     const changes = doc.getChanges();
+
     assert.ok(!changes.$unset);
     assert.deepEqual(changes, { $set: { 'myMap.abc': { myValue: 'some value' } } });
   });