From 772f6653af048064bafa30e78ea89dac8df8ae2e Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 3 Jul 2024 14:43:14 -0400 Subject: [PATCH 1/6] perf(document): avoid unnecessary set creation in $__reset() re: #14719 --- lib/document.js | 44 +++++++++++++++++++++++--------------------- lib/model.js | 1 + 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/lib/document.js b/lib/document.js index ffaa066ef58..f3882342819 100644 --- a/lib/document.js +++ b/lib/document.js @@ -3470,31 +3470,33 @@ Document.prototype.$__reset = function reset() { let _this = this; // Skip for subdocuments - const subdocs = this.$parent() === this ? this.$getAllSubdocs() : []; - 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(); + 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] = {}; + for (const array of resetArrays) { + this.$__.activePaths.clearPath(array.$path()); + array[arrayAtomicsBackupSymbol] = array[arrayAtomicsSymbol]; + array[arrayAtomicsSymbol] = {}; + } } function isParentInit(path) { diff --git a/lib/model.js b/lib/model.js index a94e3caff64..645ed78f4b0 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3209,6 +3209,7 @@ Model.$__insertMany = function(arr, options, callback) { return callback(err); } } + if (options.session != null) { doc.$session(options.session); } From 72cb0b7c7d1599b989929a4a94ea069bcc2393e8 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 3 Jul 2024 16:02:32 -0400 Subject: [PATCH 2/6] perf(document): skip parallel validate check when creating new document in insertMany() Re: #14719 --- benchmarks/insertManySimple.js | 38 ++++++++++++++++++++++++++++++++++ lib/document.js | 23 ++++++++++---------- lib/model.js | 4 +++- 3 files changed, 53 insertions(+), 12 deletions(-) create mode 100644 benchmarks/insertManySimple.js diff --git a/benchmarks/insertManySimple.js b/benchmarks/insertManySimple.js new file mode 100644 index 00000000000..9300559294e --- /dev/null +++ b/benchmarks/insertManySimple.js @@ -0,0 +1,38 @@ +'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 FooSchema = new mongoose.Schema({ foo: String }); + const FooModel = mongoose.model('Foo', FooSchema); + + if (!process.env.MONGOOSE_BENCHMARK_SKIP_SETUP) { + await FooModel.deleteMany({}); + } + + const numDocs = 1500; + const docs = []; + for (let i = 0; i < numDocs; ++i) { + docs.push({ foo: 'test foo ' + i }); + } + + const numIterations = 200; + const insertStart = Date.now(); + for (let i = 0; i < numIterations; ++i) { + await FooModel.insertMany(docs); + } + const insertEnd = Date.now(); + + const results = { + 'Average insertMany time ms': +((insertEnd - insertStart) / numIterations).toFixed(2) + }; + + console.log(JSON.stringify(results, null, ' ')); + process.exit(0); +} diff --git a/lib/document.js b/lib/document.js index f3882342819..30cf331c539 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2601,17 +2601,6 @@ Document.prototype.validate = async function validate(pathsToValidate, options) let parallelValidate; this.$op = 'validate'; - if (this.$isSubdocument != null) { - // Skip parallel validate check for subdocuments - } else if (this.$__.validating) { - parallelValidate = new ParallelValidateError(this, { - parentStack: options && options.parentStack, - conflictStack: this.$__.validating.stack - }); - } else { - this.$__.validating = new ParallelValidateError(this, { parentStack: options && options.parentStack }); - } - if (arguments.length === 1) { if (typeof arguments[0] === 'object' && !Array.isArray(arguments[0])) { options = arguments[0]; @@ -2622,6 +2611,18 @@ Document.prototype.validate = async function validate(pathsToValidate, options) const isOnePathOnly = options.pathsToSkip.indexOf(' ') === -1; options.pathsToSkip = isOnePathOnly ? [options.pathsToSkip] : options.pathsToSkip.split(' '); } + const _skipParallelValidateCheck = options && options._skipParallelValidateCheck; + + if (this.$isSubdocument != null) { + // Skip parallel validate check for subdocuments + } else if (this.$__.validating && !_skipParallelValidateCheck) { + parallelValidate = new ParallelValidateError(this, { + parentStack: options && options.parentStack, + conflictStack: this.$__.validating.stack + }); + } else if (!_skipParallelValidateCheck) { + this.$__.validating = new ParallelValidateError(this, { parentStack: options && options.parentStack }); + } if (parallelValidate != null) { throw parallelValidate; diff --git a/lib/model.js b/lib/model.js index 645ed78f4b0..f5de6fb8ead 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3199,12 +3199,14 @@ Model.$__insertMany = function(arr, options, callback) { // execute the callback synchronously return immediate(() => callback(null, doc)); } + let createdNewDoc = false; if (!(doc instanceof _this)) { if (doc != null && typeof doc !== 'object') { return callback(new ObjectParameterError(doc, 'arr.' + index, 'insertMany')); } try { doc = new _this(doc); + createdNewDoc = true; } catch (err) { return callback(err); } @@ -3220,7 +3222,7 @@ Model.$__insertMany = function(arr, options, callback) { // execute the callback synchronously return immediate(() => callback(null, doc)); } - doc.$validate().then( + doc.$validate(createdNewDoc ? { _skipParallelValidateCheck: true } : null).then( () => { callback(null, doc); }, error => { if (ordered === false) { From 69de00618e8af5b174443c8d5019f84bd4d4dbe7 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 8 Jul 2024 12:26:33 -0400 Subject: [PATCH 3/6] perf: use fast path for cloning document with only primitive values re: #14719 --- lib/document.js | 35 +++++++++++++++++++++++++++++++++-- test/document.unit.test.js | 4 +--- test/model.populate.test.js | 1 - 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/lib/document.js b/lib/document.js index 30cf331c539..2f614ffea8f 100644 --- a/lib/document.js +++ b/lib/document.js @@ -28,6 +28,7 @@ const getKeysInSchemaOrder = require('./helpers/schema/getKeysInSchemaOrder'); const getSubdocumentStrictValue = require('./helpers/schema/getSubdocumentStrictValue'); const handleSpreadDoc = require('./helpers/document/handleSpreadDoc'); const immediate = require('./helpers/immediate'); +const isBsonType = require('./helpers/isBsonType'); const isDefiningProjection = require('./helpers/projection/isDefiningProjection'); const isExclusive = require('./helpers/projection/isExclusive'); const inspect = require('util').inspect; @@ -3810,6 +3811,14 @@ Document.prototype.$toObject = function(options, json) { // `clone` is necessary here because `utils.options` directly modifies the second input. const defaultOptions = Object.assign({}, baseOptions, schemaOptions[path]); + const hasOnlyPrimitiveValues = !this.$__.populated && !this.$__.wasPopulated && (this._doc == null || Object.values(this._doc).every(v => { + return v == null + || typeof v !== 'object' + || (utils.isNativeObject(v) && !Array.isArray(v)) + || isBsonType(v, 'ObjectId') + || isBsonType(v, 'Decimal128'); + })); + // If options do not exist or is not an object, set it to empty object options = utils.isPOJO(options) ? { ...options } : {}; options._calledWithOptions = options._calledWithOptions || { ...options }; @@ -3824,7 +3833,9 @@ Document.prototype.$toObject = function(options, json) { } options.minimize = _minimize; - options._seen = options._seen || new Map(); + if (!hasOnlyPrimitiveValues) { + options._seen = options._seen || new Map(); + } const depopulate = options._calledWithOptions.depopulate ?? options._parentOptions?.depopulate @@ -3853,7 +3864,27 @@ Document.prototype.$toObject = function(options, json) { // to save it from being overwritten by sub-transform functions // const originalTransform = options.transform; - let ret = clone(this._doc, options) || {}; + let ret; + if (hasOnlyPrimitiveValues) { + // Fast path: if we don't have any nested objects or arrays, we only need a + // shallow clone. + ret = this._doc + ? options.minimize ? (minimize({ ...this._doc }) || {}) : { ...this._doc } + : {}; + if (!options.minimize || options.flattenObjectIds) { + const keys = Object.keys(ret); + for (const key of keys) { + // If `minimize` is false, still need to remove undefined keys + if (!options.minimize && ret[key] === undefined) { + delete ret[key]; + } else if (options.flattenObjectIds && isBsonType(ret[key], 'ObjectId')) { + ret[key] = ret[key].toJSON(); + } + } + } + } else { + ret = clone(this._doc, options) || {}; + } options._skipSingleNestedGetters = true; const getters = options._calledWithOptions.getters diff --git a/test/document.unit.test.js b/test/document.unit.test.js index d5b84176a35..fda93719fe4 100644 --- a/test/document.unit.test.js +++ b/test/document.unit.test.js @@ -60,8 +60,6 @@ describe('toObject()', function() { it('doesnt crash with empty object (gh-3130)', function() { const d = new Stub(); d._doc = undefined; - assert.doesNotThrow(function() { - d.toObject(); - }); + d.toObject(); }); }); diff --git a/test/model.populate.test.js b/test/model.populate.test.js index 9ad6376f89a..3583249d6eb 100644 --- a/test/model.populate.test.js +++ b/test/model.populate.test.js @@ -9449,7 +9449,6 @@ describe('model: populate:', function() { children: [{ type: 'ObjectId', ref: 'Child' }] })); - const children = await Child.create([{ name: 'Luke' }, { name: 'Leia' }]); let doc = await Parent.create({ children, child: children[0] }); From 517f27b67225c3993db0691a8a6e5563414a8fcc Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 8 Jul 2024 12:33:21 -0400 Subject: [PATCH 4/6] perf(document): only loop over doc once when shallow cloning re: #14719 --- lib/document.js | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/document.js b/lib/document.js index 2f614ffea8f..d479ca28ce8 100644 --- a/lib/document.js +++ b/lib/document.js @@ -3868,17 +3868,21 @@ Document.prototype.$toObject = function(options, json) { if (hasOnlyPrimitiveValues) { // Fast path: if we don't have any nested objects or arrays, we only need a // shallow clone. - ret = this._doc - ? options.minimize ? (minimize({ ...this._doc }) || {}) : { ...this._doc } - : {}; - if (!options.minimize || options.flattenObjectIds) { - const keys = Object.keys(ret); - for (const key of keys) { - // If `minimize` is false, still need to remove undefined keys - if (!options.minimize && ret[key] === undefined) { + if (this._doc == null) { + ret = {}; + } + ret = {}; + if (this._doc != null) { + for (const key of Object.keys(this._doc)) { + const value = this._doc[key]; + if (value instanceof Date) { + ret[key] = new Date(value); + } else if (value === undefined) { delete ret[key]; - } else if (options.flattenObjectIds && isBsonType(ret[key], 'ObjectId')) { - ret[key] = ret[key].toJSON(); + } else if (options.flattenObjectIds && isBsonType(value, 'ObjectId')) { + ret[key] = value.toJSON(); + } else { + ret[key] = value; } } } From b698c2825cc50c8cefec3a8035c8eb7dea7bb80f Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 9 Jul 2024 16:42:03 -0400 Subject: [PATCH 5/6] perf(model): skip $toObject() for insertMany with only primitive values Re: #14719 --- lib/document.js | 63 +++++++++++++++++++++++++++++-------------------- lib/model.js | 5 +++- 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/lib/document.js b/lib/document.js index 9efa9d15fa8..c67c246cd02 100644 --- a/lib/document.js +++ b/lib/document.js @@ -3811,13 +3811,7 @@ Document.prototype.$__handleReject = function handleReject(err) { Document.prototype.$toObject = function(options, json) { const defaultOptions = this.$__schema._defaultToObjectOptions(json); - const hasOnlyPrimitiveValues = !this.$__.populated && !this.$__.wasPopulated && (this._doc == null || Object.values(this._doc).every(v => { - return v == null - || typeof v !== 'object' - || (utils.isNativeObject(v) && !Array.isArray(v)) - || isBsonType(v, 'ObjectId') - || isBsonType(v, 'Decimal128'); - })); + const hasOnlyPrimitiveValues = this.$__hasOnlyPrimitiveValues(); // If options do not exist or is not an object, set it to empty object options = utils.isPOJO(options) ? { ...options } : {}; @@ -3867,27 +3861,10 @@ Document.prototype.$toObject = function(options, json) { // const originalTransform = options.transform; let ret; - if (hasOnlyPrimitiveValues) { + if (hasOnlyPrimitiveValues && !options.flattenObjectIds) { // Fast path: if we don't have any nested objects or arrays, we only need a // shallow clone. - if (this._doc == null) { - ret = {}; - } - ret = {}; - if (this._doc != null) { - for (const key of Object.keys(this._doc)) { - const value = this._doc[key]; - if (value instanceof Date) { - ret[key] = new Date(value); - } else if (value === undefined) { - delete ret[key]; - } else if (options.flattenObjectIds && isBsonType(value, 'ObjectId')) { - ret[key] = value.toJSON(); - } else { - ret[key] = value; - } - } - } + ret = this.$__toObjectInternal(); } else { ret = clone(this._doc, options) || {}; } @@ -3948,6 +3925,26 @@ Document.prototype.$toObject = function(options, json) { return ret; }; +/*! + * Internal shallow clone alternative to `$toObject()`: much faster, no options processing + */ + +Document.prototype.$__toObjectInternal = function $__toObjectInternal() { + const ret = {}; + if (this._doc != null) { + for (const key of Object.keys(this._doc)) { + const value = this._doc[key]; + if (value instanceof Date) { + ret[key] = new Date(value); + } else if (value !== undefined) { + ret[key] = value; + } + } + } + + return ret; +}; + /** * Converts this document into a plain-old JavaScript object ([POJO](https://masteringjs.io/tutorials/fundamentals/pojo)). * @@ -5328,6 +5325,20 @@ Document.prototype.$clearModifiedPaths = function $clearModifiedPaths() { return this; }; +/*! + * Check if the given document only has primitive values + */ + +Document.prototype.$__hasOnlyPrimitiveValues = function $__hasOnlyPrimitiveValues() { + return !this.$__.populated && !this.$__.wasPopulated && (this._doc == null || Object.values(this._doc).every(v => { + return v == null + || typeof v !== 'object' + || (utils.isNativeObject(v) && !Array.isArray(v)) + || isBsonType(v, 'ObjectId') + || isBsonType(v, 'Decimal128'); + })); +}; + /*! * Module exports. */ diff --git a/lib/model.js b/lib/model.js index a2cf862a676..4d2e9a3aad9 100644 --- a/lib/model.js +++ b/lib/model.js @@ -2951,7 +2951,10 @@ Model.$__insertMany = function(arr, options, callback) { } const shouldSetTimestamps = (!options || options.timestamps !== false) && doc.initializeTimestamps && (!doc.$__ || doc.$__.timestamps !== false); if (shouldSetTimestamps) { - return doc.initializeTimestamps().toObject(internalToObjectOptions); + doc.initializeTimestamps(); + } + if (doc.$__hasOnlyPrimitiveValues()) { + return doc.$__toObjectInternal(); } return doc.toObject(internalToObjectOptions); }); From 8223f91fbb6738c625fc99c671c2c4035b7274c6 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 10 Jul 2024 11:44:13 -0400 Subject: [PATCH 6/6] refactor: rename to $__toObjectShallow() based on code review comments --- lib/document.js | 4 ++-- lib/model.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/document.js b/lib/document.js index aee4e6d6772..c4afeb763a7 100644 --- a/lib/document.js +++ b/lib/document.js @@ -3866,7 +3866,7 @@ Document.prototype.$toObject = function(options, json) { if (hasOnlyPrimitiveValues && !options.flattenObjectIds) { // Fast path: if we don't have any nested objects or arrays, we only need a // shallow clone. - ret = this.$__toObjectInternal(); + ret = this.$__toObjectShallow(); } else { ret = clone(this._doc, options) || {}; } @@ -3931,7 +3931,7 @@ Document.prototype.$toObject = function(options, json) { * Internal shallow clone alternative to `$toObject()`: much faster, no options processing */ -Document.prototype.$__toObjectInternal = function $__toObjectInternal() { +Document.prototype.$__toObjectShallow = function $__toObjectShallow() { const ret = {}; if (this._doc != null) { for (const key of Object.keys(this._doc)) { diff --git a/lib/model.js b/lib/model.js index 4d2e9a3aad9..9cd7a14d6de 100644 --- a/lib/model.js +++ b/lib/model.js @@ -2954,7 +2954,7 @@ Model.$__insertMany = function(arr, options, callback) { doc.initializeTimestamps(); } if (doc.$__hasOnlyPrimitiveValues()) { - return doc.$__toObjectInternal(); + return doc.$__toObjectShallow(); } return doc.toObject(internalToObjectOptions); });