From 4f222235ff0bde74fbdcfebed371803427a66a96 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 16 Apr 2023 13:02:01 -0400 Subject: [PATCH 1/6] perf(document): avoid some unnecessary work when checking if map paths are modified Re: #13191 --- lib/document.js | 20 +++++++++++++++----- lib/types/map.js | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/lib/document.js b/lib/document.js index 7978dcb4bf6..179b1ce8dca 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2217,14 +2217,21 @@ Document.prototype[documentModifiedPaths] = Document.prototype.modifiedPaths; Document.prototype.isModified = function(paths, modifiedPaths) { if (paths) { - const directModifiedPaths = Object.keys(this.$__.activePaths.getStatePaths('modify')); + if (!Array.isArray(paths)) { + paths = paths.indexOf(' ') === -1 ? [paths] : paths.split(' '); + } + + const directModifiedPathsObj = this.$__.activePaths.getStatePaths('modify'); + const directModifiedPaths = Object.keys(directModifiedPathsObj); if (directModifiedPaths.length === 0) { return false; } - - if (!Array.isArray(paths)) { - paths = paths.indexOf(' ') === -1 ? [paths] : paths.split(' '); + for (const path of paths) { + if (directModifiedPathsObj.hasOwnProperty(path)) { + return true; + } } + const modified = modifiedPaths || this[documentModifiedPaths](); const isModifiedChild = paths.some(function(path) { return !!~modified.indexOf(path); @@ -3317,7 +3324,10 @@ Document.prototype.$__reset = function reset() { let _this = this; // Skip for subdocuments - const subdocs = this.$parent() === this ? this.$getAllSubdocs() : []; + if (this.$isDocumentArrayElement || this.$isSingleNested) { + return this; + } + const subdocs = this.$getAllSubdocs(); const resetArrays = new Set(); for (const subdoc of subdocs) { const fullPathWithIndexes = subdoc.$__fullPathWithIndexes(); diff --git a/lib/types/map.js b/lib/types/map.js index 2004dd4ad3e..11ba313d89e 100644 --- a/lib/types/map.js +++ b/lib/types/map.js @@ -141,12 +141,12 @@ class MongooseMap extends Map { super.set(key, value); if (value != null && value.$isSingleNested) { - value.$basePath = this.$__path + '.' + key; + value.$basePath = fullPath; } const parent = this.$__parent; if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) { - parent.markModified(this.$__path + '.' + key); + parent.markModified(fullPath); } } From 51b898248034ea8b0552eafe0190da7fbdd4db74 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 16 Apr 2023 14:01:45 -0400 Subject: [PATCH 2/6] perf(document): fix build, avoid unnecessary Object.keys() re: #13191 --- lib/document.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/document.js b/lib/document.js index 179b1ce8dca..054707d9b81 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2221,9 +2221,8 @@ Document.prototype.isModified = function(paths, modifiedPaths) { paths = paths.indexOf(' ') === -1 ? [paths] : paths.split(' '); } - const directModifiedPathsObj = this.$__.activePaths.getStatePaths('modify'); - const directModifiedPaths = Object.keys(directModifiedPathsObj); - if (directModifiedPaths.length === 0) { + const directModifiedPathsObj = this.$__.activePaths.states.modify; + if (directModifiedPathsObj == null) { return false; } for (const path of paths) { @@ -2237,6 +2236,7 @@ Document.prototype.isModified = function(paths, modifiedPaths) { 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 + '.'); @@ -3324,10 +3324,7 @@ Document.prototype.$__reset = function reset() { let _this = this; // Skip for subdocuments - if (this.$isDocumentArrayElement || this.$isSingleNested) { - return this; - } - const subdocs = this.$getAllSubdocs(); + const subdocs = this.$parent() === this ? this.$getAllSubdocs() : []; const resetArrays = new Set(); for (const subdoc of subdocs) { const fullPathWithIndexes = subdoc.$__fullPathWithIndexes(); From 7bec5064d0a39032192be93df53e832f948207d8 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 16 Apr 2023 14:59:21 -0400 Subject: [PATCH 3/6] perf(map): delay string concatenation unless necessary Re: #13191 --- lib/types/map.js | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/lib/types/map.js b/lib/types/map.js index 11ba313d89e..71c957d9ac4 100644 --- a/lib/types/map.js +++ b/lib/types/map.js @@ -96,9 +96,9 @@ class MongooseMap extends Map { return; } - const fullPath = this.$__path + '.' + key; + let _fullPath; const populated = this.$__parent != null && this.$__parent.$__ ? - this.$__parent.$populated(fullPath, true) || this.$__parent.$populated(this.$__path, true) : + this.$__parent.$populated(this.$__path, true) : null; const priorVal = this.get(key); @@ -127,11 +127,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 ? + { 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; @@ -141,12 +149,22 @@ class MongooseMap extends Map { super.set(key, value); if (value != null && value.$isSingleNested) { - value.$basePath = fullPath; + value.$basePath = fullPath.call(this); } const parent = this.$__parent; if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) { - parent.markModified(fullPath); + parent.markModified(fullPath.call(this)); + } + + // 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; } } From e6eec8734bb314adc2c689a5052ce5f8004b4784 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 16 Apr 2023 15:45:36 -0400 Subject: [PATCH 4/6] test: fix tests re: #13191 --- lib/types/map.js | 12 ++++-------- test/types.map.test.js | 1 + 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/lib/types/map.js b/lib/types/map.js index 71c957d9ac4..c6510d49bc1 100644 --- a/lib/types/map.js +++ b/lib/types/map.js @@ -97,8 +97,9 @@ class MongooseMap extends Map { } let _fullPath; - const populated = this.$__parent != null && this.$__parent.$__ ? - this.$__parent.$populated(this.$__path, true) : + 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,7 +128,7 @@ class MongooseMap extends Map { } } else { try { - const options = this.$__schemaType.$isMongooseDocumentArray ? + const options = this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested ? { path: fullPath.call(this) } : null; value = this.$__schemaType.applySetters( @@ -148,11 +149,6 @@ class MongooseMap extends Map { super.set(key, value); - if (value != null && value.$isSingleNested) { - value.$basePath = fullPath.call(this); - } - - const parent = this.$__parent; if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) { parent.markModified(fullPath.call(this)); } diff --git a/test/types.map.test.js b/test/types.map.test.js index bff74800c02..c08482819d2 100644 --- 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' } } }); }); From 96890eceb77efa9c099145a86bbea0d3348aca98 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 18 Apr 2023 13:10:48 -0400 Subject: [PATCH 5/6] chore: add benchmark for #13191 --- benchmarks/mapOfSubdocs.js | 55 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 benchmarks/mapOfSubdocs.js diff --git a/benchmarks/mapOfSubdocs.js b/benchmarks/mapOfSubdocs.js new file mode 100644 index 00000000000..9fc6e401919 --- /dev/null +++ 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, ' ')); +} \ No newline at end of file From b9ca7ca4dbe53fcad194a5bd138dbf87b6ee815a Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 18 Apr 2023 13:11:13 -0400 Subject: [PATCH 6/6] fix: use Object.prototype.hasOwnProperty re: code review --- lib/document.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/document.js b/lib/document.js index 054707d9b81..84ddc7c2acc 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2226,7 +2226,7 @@ Document.prototype.isModified = function(paths, modifiedPaths) { return false; } for (const path of paths) { - if (directModifiedPathsObj.hasOwnProperty(path)) { + if (Object.prototype.hasOwnProperty.call(directModifiedPathsObj, path)) { return true; } }