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: speed up creating maps of subdocuments #13280

Merged
merged 6 commits into from Apr 27, 2023
Merged

Conversation

vkarpov15
Copy link
Collaborator

Summary

Re: #13191, #13271.

Benchmark from #13191 with 2000 documents, before this change:

Starting the process of mapping all minis and timing it
Iterating through all minis: 5.661ms
Sorting all minis: 1.368ms
Creating and populating Map of Minis: 0.4ms
Saving Map of Minis to MongoDB using Mongoose: 1.684s
Map Process: 1.692s
Saved Minis Map successfully!

After:

Starting the process of mapping all minis and timing it
Iterating through all minis: 6.554ms
Sorting all minis: 1.872ms
Creating and populating Map of Minis: 0.555ms
Saving Map of Minis to MongoDB using Mongoose: 315.028ms
Map Process: 324.444ms
Saved Minis Map successfully!

I would love to make this faster and will work on it some more, as well as adding a benchmark for this, but figured I'd add a PR first.

Examples

@vkarpov15 vkarpov15 requested a review from hasezoey April 16, 2023 19:02
lib/document.js Outdated Show resolved Hide resolved
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 + '.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just calling startsWith is more ergonomic than making the direct comparison.
Or maybe store path length in a variable and then do someting like

return mod.length === pathLength && mod === path || mod.length > pathLength && mod[pathLength] === "." && path.startsWith(mod)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do this we would need to put it into a for loop. We could theoretically do directly a for of loop of Object.keys as Object.keys returns an iterator and it should not make a perf difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a bad idea, but I'd like to do some additional benchmarking to see if this makes a meaningful difference. We'll keep #13191 open for future work related to this.

Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

@vkarpov15 vkarpov15 added this to the 6.10.6 milestone Apr 18, 2023
@vkarpov15 vkarpov15 merged commit e364017 into 6.x Apr 27, 2023
34 checks passed
@hasezoey hasezoey deleted the vkarpov15/gh-13191 branch April 27, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants