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

feat(reactivity): ref-specific track/trigger and miscellaneous optimizations #3995

Merged
merged 2 commits into from
Jun 23, 2021

Conversation

basvanmeurs
Copy link
Contributor

New PR created, see #2345.

performance-3 1 1
performance-PR

@yyx990803 yyx990803 added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. scope: reactivity version: minor labels Jun 23, 2021
@yyx990803
Copy link
Member

I noticed this PR no longer use a class/prototype implementation for effects - does this mean this PR doesn't affect memory consumption like #2345 did?

@yyx990803
Copy link
Member

yyx990803 commented Jun 23, 2021

Also curious why there is such a big improvement in creation of watchers / reactive objects - it seems unrelated to this PR? Or are you running the benchmarks on a branch that contains more changes than this PR?

Ran the benchmarks myself and indeed seeing same improvements - the watch creation seems to have to do with the ref read. Although I'm still amazed that simple reactive({}) call somehow got faster because of this PR 😅

@yyx990803 yyx990803 merged commit 44a1d39 into vuejs:3.2 Jun 23, 2021
@yyx990803
Copy link
Member

This is great! Thank you!

Do you still plan to work on the memory-saving aspect and the dep cleanup optimizations (described by @jods4 here)?

@basvanmeurs
Copy link
Contributor Author

basvanmeurs commented Jun 24, 2021

Although I'm still amazed that simple reactive({}) call somehow got faster because of this PR

I don't think it got faster. In theory, the changes in this PR shouldn't affect it at all. That particular benchmark just gives very different result every run for some reason..

I noticed this PR no longer use a class/prototype implementation for effects - does this mean this PR doesn't affect memory consumption like #2345 did?

That's correct. I left them out, because the memory consumption changes require a breaking API change for the reactivity module. These were roughly the optimizations that could be done:

  1. moving lazy, allowRecurse flags from options to function argument
  2. moving the scheduler function
  3. use EMPTY_OBJ as options object when there are no options
  4. use prototype implementation to allow us to place less-frequently modified variables on the prototype.

Notice that most gain can be obtained from 1-3, and these only affect the arguments in the effect function. 4 has a wider impact on the API because effect itself can no longer be a function, the actual function must be moved to a prototype property.

Are you prepared to change the reactivity API at all? And to what extent?

@basvanmeurs
Copy link
Contributor Author

basvanmeurs commented Jun 24, 2021

Do you still plan to work on the memory-saving aspect and the dep cleanup optimizations (described by @jods4 here)?

I actually tried to avoid set add/removes and sync them after the effect (https://github.com/Planning-nl/vue-next/commit/f8cc160aed8c581662629904457f4a87437e62d5). In most tests, performance is boosted by ~10 to 20%.

However, some tests fail because it seems that, in some cases, synchronized behavior from within the effect require the dependencies (Set) to be up to date at any time. When postponing dep registration until after the track op has finished, it seems that some use cases aren't properly covered.

Any way, imho the additional complexity is too much and the performance gains too minimal to actually implement this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. scope: reactivity version: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants