-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
feat(effect): refactorings to improve performance, reduce memory usage #2345
Conversation
I think we need to keep track of the performance of the vue core, especially the reactivity part. To verify my own work, as well as testing for future development, I created this little repository: https://github.com/basvanmeurs/vue-next-benchmarks You can run some reactivity benchmarks live here: https://basvanmeurs.github.io/vue-next-benchmarks/ You can either enter a specific version or a specific url. The work in this PR can be compared to the current release using the following URLs: |
This is great work! However, it seems it contains some non trivial changes to the usage of Regarding the benchmarks - this is very helpful, it'd be awesome to maybe extend this to component instance creation and vdom updates as well. We've had the thought of running something similar on CI but never got time to actually work on it. |
It would be very great to implement #2195 after this. |
Reactivity API changes
Vue API changesThe external vue api has not changed as the |
Moving back to draft as I have some additional optimizations |
|
||
ReactiveEffect.prototype.active = true | ||
ReactiveEffect.prototype.allowRecurse = false | ||
ReactiveEffect.prototype.options = EMPTY_OBJ |
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.
@basvanmeurs The code might be cleaner if you declare those three as getters on the class?
public get active() { return true }
would end up on the prototype and I don't think usage of a getter rather than field would impact perf / memory in any noticeable way (we have your benchmarks to verify).
Other than coding style, it's also easier for tools to determine that a module is side-effect free if it doesn't contain statements, just class declarations.
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.
I was initially enthousiastic. But when I tried changing this, all property assignments started to cause typescript errors (Cannot assign to 'options' because it is a read-only property.). I could ignore them for every assignment by casting to any: (this as any).options = {}
. But personally I think that the resulting code will be worse than what it is now.
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.
Ah, I did not see that one coming.
I agree your original code is a lot better than using any
in callers.
I see no way to "idiomatically" have TS infer that we're having fields with default values stored in prototype.
If we did things in a "less-JS" way we would either mark those fields optional and provide the default value in code that reads them, or always init fields with the default value.
Not sure about the perf/memory impact of those ways, it would need some benchmarking.
Always assigning a value to a field member would increase memory usage but might lead to monomorphic code, which maybe executes faster. Not sure if it would be noticeable. You have a benchmark avail., maybe you could try.
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.
I tried setting the properties in the constructor during development for the reason you mentioned. It did not affect performance at all, at least in Chromium. It might be different in other browsers, but probably not by much.
Because it really saved some memory I decided to put them on the prototype.
@basvanmeurs Thank you very much for tackling this. ❤️ Like you, I noticed there was room for improvement in the reactivity package and wanted to create a PR. Couldn't manage to actually do it because I had too much work. During a run, the dependency tracking logic is simply throwing out all previous dependencies, and builds up new arrays, maps, with the detected dependencies. I see you've optimized those, but it's still a non-trivial amount of allocations. A key observation is that most effects have constant, or nearly constant, list of dependencies. So prior art uses a different way to update those dependency lists:
|
Looked at your code and there's another thing that you could do on top of my previous comment.
When a trigger effect runs, other effects may be triggered in cascade and run on top of it, possibly adding or removing themselves in the dependency list currently iterated. I'll explain why the "concept" of adding or removing dependencies doesn't matter:
Removed dependencies can create a mutation problem in the array, though. If we're at index This can be solved by not splicing removed dependencies, but rather setting them to null. That way, the array never shrinks and iteration won't skip any element. One short-term benefit here is that setting an array item to null is more efficient than splicing, which must copies all subsequent items and is O(N). One problem is that now we may have holes in the array and over time that may become a memory leak if a dependency is added/removed in a cycling fashion.
All of this might sound like it's a bit tricky, but if you abstract dependencies list management into a single class or module or derived array then it's self contained and well encapsulated, calling code is clean. |
Hi @jods4 Thanks for your review and feedback! I also realized the importance of improving the performance on this, as in my own app I found that too much time was spent in triggering and tracking stuff. Let me first respond to your first idea (effect ids).
That's a claim that can be easily investigated with my vue-next-benchmarks ;-) I've used my own PR for benchmarking. BenchmarksThe 'computed:write ref, read 1000 computeds' is a good test to see the impact of this tracking/triggering in practice. The code is like this: () => {
const v = ref(100);
const computeds = [];
for (let i = 0, n = 1000; i < n; i++) {
const c = computed(() => {
return v.value * 2
});
const cv = c.value;
computeds.push(c);
}
let i = 0;
return suite.add("write ref, read 1000 computeds", () => {
v.value = i++;
computeds.forEach(c => c.value);
});
} If the total time, about 30% of the time is spent in the 'cleanup' of effects, and 18% (surprisingly less than the cleanup) is spent in trackRefValue, and 3% is spent in triggerRefValue. More than half of the total time is spent on the process that you described. GC only takes about 1 to 2% by the way. Chrome is very good at reusing those many small temporary arrays and sets efficiently. What if we reverse the test, having a single computed having many (1000) dependencies? () => {
const refs = [];
for (let i = 0, n = 1000; i < n; i++) {
refs.push(ref(i));
}
const c = computed(() => {
let total = 0;
refs.forEach(ref => total += ref.value);
return total;
});
let i = 0;
const n = refs.length;
return suite.add("1000 refs, 1 computed", () => {
refs[i++ % n].value++;
const v = c.value;
});
} The test is twice as fast as the first one. But Looking towards reactive objects (reactiveObject:write reactive obj, read 1000 computeds): () => {
const r = reactive({a: 1});
const computeds = [];
for (let i = 0, n = 1000; i < n; i++) {
const c = computed(() => {
return r.a * 2
});
computeds.push(c);
}
let i = 0;
return suite.add("write reactive obj, read 1000 computeds", () => {
r.a = i++;
computeds.forEach(c => c.value);
});
} There are 1000 computeds referencing 1 ref. The figures here are 18% cleanup, 17% tracking, 6% triggering. In comparison, tracking/triggering is relatively more expensive because of the dependency sub-set selection. And the relative time spent in the tracking/triggering is less because the proxy in itself causes a lot of overhead as well. Still it amounts to 41%. Profiling results for the Reactivity benchmark conclusionsSo I think we can roughly gain about 50% performance by preventing the deps cleanup/retrack process. Very significant and way too much to ignore if you ask me.. Especially as it's, as you mentioned, at the heart of everything. As GC time is not significant, I think we shouldn't focus on prevent assigning new Sets and arrays. We should be focusing on the idea of preventing tracking and cleanup, or at least making it smarter. Then re-run the performance tests to see where we're at :-) Cleanup performanceYou mention some interesting ideas. To tell you the truth I had an idea of my own, but it relied on the order of tracked effects which complicated things. Will your ideas help performance? I believe that even checking whether an effect already exists in a set takes a similar amount of time than to really add it! So your idea will not help us much in terms of tracking performance. But not having to cleanup beforehand will really help a lot based on the profiling information. Up to 20 to 30%. Another risk is introducing (hard to find) bugs. Your algorithm is simple, but we have to take into account that effects are triggered recursively. So during the run of an effect, other effects will be ran as well, and the run id is incremented and decremented multiple times before the cleanup takes place. Let's say we have effect A. It's ran, so we record the current run id and invoke the effect. In the case that one of the original effects (let's call it B) of effect A is no longer used by A, but is still used by one of the recursively triggered effects of A, B will still be registered as a dependency of A as it's run id is higher than A's run id. The end result will be that A 'falsely' retains its dependency to B. We could fix this by maintaining an array of run ids but this has (performance and complexity) downsides. So let's take a step back: is it a problem that A retains the dependency to B? First off, this is really an edge case. And as far as I can tell, this is not really a problem because when that dependency is triggered, it would recursively trigger the subject as well anyway. It won't lead to memory leaks as it will be removed once it is no longer a dependency of a dependency of A. This idea is quite easy to implement and test. It will cost only a couple of lines of code, and gain us 20 to 30% of performance. I think that's worth it. Having said all that, I must also share my disappointment. After the release of vue-next, it seems that all progress on the core has come to a halt. I'm reluctant to spend any time on implementing this or other improvements right now, as long as this PR is not merged. I would only be making the merge more difficult if I would. And there's no guarantee that it will be merged at all.. This project is great but this is the time to streamline the core. If we don't do it now it will become more and more difficult. |
To clarify: run id is a monotone sequence, always increasing. Effects are not re-entrant. The code checks if the effect is already on the stack and doesn't run it a second time. |
Reduces memory usage for (computed) refs by 25% and improves creation performance
Move out of the ReactiveEffectOptions in an effort to remove the necessity of creating this object for most use cases, saving memory and performance.
Move several options to params, so that an object is not required for watchers or computeds. This reduces memory. On top of that, place some optional params on the prototype unless non-default.
There's no point in underscoring the effect property of the runner.
…body 1. One variable less on the stack 2. Smaller functions means less time to optimize
Out-of-bounds should be avoided always, for performance reasons. In practice, some benchmarks (especially 'write ref, read 1000 computeds') showed significant (30%) improvement after this change.
Explain that refs store their deps in a local property.
Improves performance.
@yyx990803 would you prefer a new PR which only includes the performance improvements that don't affect the API? |
@basvanmeurs yes, I'm actually reviewing the changes for 3.2 right now and it would be great to have a new PR with a perf-only changeset (and rebased). Also, I apologize for letting this sit for so long - and really appreciate your patience. Thanks again! |
Thanks Evan. See #3995 |
Based on #2345 , but with smaller API change - Use class implementation for `ReactiveEffect` - Switch internal creation of effects to use the class constructor - Avoid options object allocation - Avoid creating bound effect runner function (used in schedulers) when not necessary. - Consumes ~17% less memory compared to last commit - Introduces a very minor breaking change: the `scheduler` option passed to `effect` no longer receives the runner function.
Based on #2345 , but with smaller API change - Use class implementation for `ReactiveEffect` - Switch internal creation of effects to use the class constructor - Avoid options object allocation - Avoid creating bound effect runner function (used in schedulers) when not necessary. - Consumes ~17% less memory compared to last commit - Introduces a very minor breaking change: the `scheduler` option passed to `effect` no longer receives the runner function.
Based on #2345 , but with smaller API change - Use class implementation for `ReactiveEffect` - Switch internal creation of effects to use the class constructor - Avoid options object allocation - Avoid creating bound effect runner function (used in schedulers) when not necessary. - Consumes ~17% less memory compared to last commit - Introduces a very minor breaking change: the `scheduler` option passed to `effect` no longer receives the runner function.
Based on #2345 , but with smaller API change - Use class implementation for `ReactiveEffect` - Switch internal creation of effects to use the class constructor - Avoid options object allocation - Avoid creating bound effect runner function (used in schedulers) when not necessary. - Consumes ~17% less memory compared to last commit - Introduces a very minor breaking change: the `scheduler` option passed to `effect` no longer receives the runner function.
Based on #2345 , but with smaller API change - Use class implementation for `ReactiveEffect` - Switch internal creation of effects to use the class constructor - Avoid options object allocation - Avoid creating bound effect runner function (used in schedulers) when not necessary. - Consumes ~17% less memory compared to last commit - Introduces a very minor breaking change: the `scheduler` option passed to `effect` no longer receives the runner function.
Based on #2345 , but with smaller API change - Use class implementation for `ReactiveEffect` - Switch internal creation of effects to use the class constructor - Avoid options object allocation - Avoid creating bound effect runner function (used in schedulers) when not necessary. - Consumes ~17% less memory compared to last commit - Introduces a very minor breaking change: the `scheduler` option passed to `effect` no longer receives the runner function.
This PR is a refactoring of the reactivity module.
Results
Performance
You can compare my benchmarks on your own machine:
This PR
3.0.0
Or view my own results:
Memory
In terms of memory I only did a pretty rudimentary test:
I managed to save some memory by using more properties on the prototypes, and moved some parameters from the option object because those ReactiveEffectOptions objects have a memory overhead as well. Left is the PR in its current state, right is 3.0.0. A reduction of about 30%. I think we're approaching the limit of what we can do.