Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document $__undoReset performance on deeply nested documents #15255

Closed
2 tasks done
mcat95 opened this issue Feb 13, 2025 · 0 comments · Fixed by #15257
Closed
2 tasks done

Document $__undoReset performance on deeply nested documents #15255

mcat95 opened this issue Feb 13, 2025 · 0 comments · Fixed by #15257
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. performance
Milestone

Comments

@mcat95
Copy link

mcat95 commented Feb 13, 2025

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Mongoose version

8.10.0

Node.js version

v18.19.0

MongoDB server version

8.0.4

Typescript version (if applicable)

No response

Description

When updating a document fails due to a version error, we've noticed that the method $__undoReset is called for the document. However, we found that it gets called recursively many more times than needed, because it's calling getAllSubdocs (which should get all subdocs on the current document regardless of how deeply nested they are) recursively, which causes the undoReset function to be called a lot.

On our repro, adding a console.count to the header of the $__undoReset function, the function gets called around 700k times, taking a second or so to execute. However, the sample document only contains around 1024 subdocuments.

Steps to Reproduce

const mongoose = require('mongoose');

const RecursiveSchema = new mongoose.Schema({});

const s = [ RecursiveSchema ];
RecursiveSchema.path('nested', s);

const generateRecursiveDocument = (depth, curr = 0) => {
  return {
    name: `Document of depth ${curr}`,
    nested: depth > 0 ? new Array(2).fill().map(() => generateRecursiveDocument(depth-1, curr+1)) : [],
    __v: 5,
  };
}

const main = async () => {
  console.log(mongoose.version)
  await mongoose.connect('mongodb://localhost/mongoose-test');
  const model = new mongoose.model('Recursive', RecursiveSchema);
  const data = generateRecursiveDocument(10);
  const doc = new model(data);
  await doc.save();

  const d = await model.findById(doc._id);
  d.increment();
  d.data = 'asd';
  // Force a version error by updating the document directly
  await model.collection.updateOne({_id: doc._id}, {$inc: { __v: 1}})
  console.time('Save');
  await d.save().catch(() => {});
  console.timeEnd('Save');

};

main().then(process.exit);

Expected Behavior

This should be way faster, because the undoReset should only get called once per subdocument. We've made a quick workaround plugin that overrides the original undoReset method and makes it not recursive, since getAllSubdocs is already recursive:

  const arrayAtomicsBackupSymbol = require('mongoose/lib/helpers/symbols').arrayAtomicsBackupSymbol;
  const arrayAtomicsSymbol = require('mongoose/lib/helpers/symbols').arrayAtomicsSymbol;

  function undoResetDocPlugin(schema) {
    function undoResetDoc(doc) {
      if (doc.$__.backup == null || doc.$__.backup.activePaths == null) {
        return;
      }

      doc.$__.activePaths.states.modify = doc.$__.backup.activePaths.modify;
      doc.$__.activePaths.states.default = doc.$__.backup.activePaths.default;

      doc.$__.validationError = doc.$__.backup.validationError;
      doc.$errors = doc.$__.backup.errors;

      for (const dirt of doc.$__dirty()) {
        const type = dirt.value;

        if (type && type[arrayAtomicsSymbol] && type[arrayAtomicsBackupSymbol]) {
          type[arrayAtomicsSymbol] = type[arrayAtomicsBackupSymbol];
        }
      }
    }
    schema.methods.$__undoReset = function() {
      undoResetDoc(this);
      for (const subdoc of this.$getAllSubdocs()) {
        undoResetDoc(subdoc);
      };
    };
  }

  RecursiveSchema.plugin(undoResetDocPlugin);
@vkarpov15 vkarpov15 added performance confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. labels Feb 13, 2025
@vkarpov15 vkarpov15 added this to the 8.10.1 milestone Feb 13, 2025
vkarpov15 added a commit that referenced this issue Feb 14, 2025
perf(document): only call `undoReset()` 1x/document
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants