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(document): avoid creating unnecessary empty objects when creating a state machine #11988
Conversation
…g a state machine Re: #11541
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. 👍
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
lib/connection.js
Outdated
if (doc.$__.activePaths.states.modify == null) { | ||
doc.$__.activePaths.states.modify = {}; | ||
} |
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 i understand this check correctly, then this could be done out of the loop (for (const path of state.modifiedPaths)
) because it does not need to be repeated, only a condition would need to be added to check that state.modifiedPaths
is not empty as to not unnecessary set it when empty
i did a small test with performance across 3 different points with 3 runs each:
// first i ran on branch 6.5, where
memavg = 147.31509908040366
timeavg = 1944.6666666666667
// then i ran on branch vkarpov15/gh-11541, where
memavg = 120.52366638183594
timeavg = 1887.3333333333333
// then i modified and moved the if outside of the mentioned loop, where
memavg = 121.06150309244792
timeavg = 1834.3333333333333
Run Numbers
run1mem=[145.24718475341797,151.04356384277344,145.65454864501953]
run1time=[1938,1952,1944]
run2mem=[119.71839141845703,120.27562713623047,121.57698059082031]
run2time=[1929,1861,1872]
run3mem=[120.03515625,122.43016052246094,120.71919250488281]
run3time=[1812,1849,1842]
sum = (arr) => {let sum=0;for(const a of arr) {sum+=a} return sum;}
avg = (arr) => {let asum = sum(arr); return (asum / arr.length) || 0;}
avg(run1mem);
avg(run1time);
avg(run2mem);
avg(run2time);
avg(run3mem);
avg(run3time);
Note: the if i used is:
if (state.modifiedPaths.length > 0 && doc.$__.activePaths.states.modify == null) {
doc.$__.activePaths.states.modify = {};
}
and i had also modified the test script a bit: https://gist.github.com/hasezoey/6bc112e760b05e9a059550aa8e3dbbfa
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.
Good suggestion 👍
Re: #11541
Summary
Every document gets a state machine instance in
doc.$__.activePaths
to track whether a path is modified, required, init, ignored, etc. However, this can be a memory hog if there are no paths that are in a given state, especially if we have 10k+ subdocuments.Here's the benchmark script I've been using for #11541:
Before this change:
After:
So this represent a decent improvement in memory usage that I'd like to merge in 6.5. There's some risk because
Object.keys(null)
throws an error, and we've historically usedObject.keys(this.$__.activePaths.states.modify)
to get all modified paths. There may be plugins that copied our code that would need to update.Examples