-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
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 + '.'); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! 👍
Summary
Re: #13191, #13271.
Benchmark from #13191 with 2000 documents, before this change:
After:
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