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

perf: cache results from getAllSubdocs() on saveOptions, only loop through known subdoc properties #15055

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

vkarpov15
Copy link
Collaborator

Fix #15029

Summary

#15029 points out that we spend a lot of unnecessary CPU cycles in save() in $getAllSubdocs(), even for schemas that have no subdocs. That's because currently save() calls $getAllSubdocs() 5 times, and $getAllSubdocs() loops through every path in the document searching for subdocs.

With this PR, we will only call $getAllSubdocs() 1x if calling save() with no subdocs, and also $getAllSubdocs() will only loop through paths we know can have subdocs based on the schema. So if the schema doesn't have any subdocuments, document arrays, or maps of subdocs, then $getAllSubdocs() will have an empty for loop.

In my quick experiment with the saveSimple benchmark, I'm seeing a 2.7% improvement in performance, which isn't huge, but that's 2.7% on every save call.

I'll also work on adding a benchmark with subdocuments to see how much this helps with subdocs.

Examples

@vkarpov15
Copy link
Collaborator Author

Quick check with the createDeepNestedDocArray benchmark, which is a good example of save() with a lot of very deeply nested subdocs, shows a 5% speedup.

@billouboq
Copy link
Contributor

Does it improves performance of create or insertMany ?

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, aside from the minor jsdoc change necessary. I would recommend to put this into the next minor version for caution.

benchmarks/saveSimple.js Show resolved Hide resolved
lib/document.js Show resolved Hide resolved
@hasezoey
Copy link
Collaborator

@billouboq

Does it improves performance of create or insertMany ?

It will affect any function making use of $getAllSubdocs, which by extension is $validate (which is then made use of in insertMany, save, etc), thought to different extends depending on previous amount of calls to $getAllSubdocs.
(note that create is using .save, so any changes to save also affects create directly).

@vkarpov15 vkarpov15 added this to the 8.8.3 milestone Nov 25, 2024
@vkarpov15
Copy link
Collaborator Author

insertMany() uses validate() under the hood, so this should help a bit on insertMany() too. Definitely on create(), because create() calls save().

@vkarpov15 vkarpov15 merged commit 862d1a5 into master Nov 26, 2024
72 of 73 checks passed
@vkarpov15 vkarpov15 deleted the vkarpov15/gh-15029 branch November 26, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants