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): bitwise dep markers to optimize re-tracking #4017

Merged
merged 1 commit into from
Jul 7, 2021
Merged

feat(reactivity): bitwise dep markers to optimize re-tracking #4017

merged 1 commit into from
Jul 7, 2021

Conversation

basvanmeurs
Copy link
Contributor

Currently for every effect run, all existing dependencies are first cleaned up and then retracked. This involves Set additions and removals. In many cases, dependencies rarely change so we already discussed that there was room for optmization here (#2345 (comment)).

This PR optimizes re-tracking of effect dependencies.

It uses markers which identify whether a dependency is new, stable or old. Because effect execution/tracking may be recursive, the markers are actually bitwise and support up to 30 levels deep (the amount of bits we can safely use for a SMI small integer). When the depth is more than 30 levels (notice that this is unlikely to occur), the 'old' full cleanup method is used instead.

Component render/update recursion

Notice that this optimization, unlike the old cleanup/retrack method, postpones deleting the old dependencies until the end of the effect execution. Ideally, this should not make a difference, but I found one situation in which it did: the component update.

The onBeforeUpdate test in apiLifecycle.spec.ts failed. It turned out that the component update 'effect' has the allowRecursive flag turned on, because child components must be able to re-trigger the parent. However, when one of the (beforeMount/beforeUpdate) life cycle hooks is invoked, it is possible that this triggers any of the dependencies of the render method. This causes the render/update effect to be queued again unnecessarily.

Currently, the 'cleanup' cleared all dependencies just in time, making sure that this recursive trigger doesn't occur (I don't think this is intentional, it rather feels like a lucky side effect to me). To make this PR work, I had to explicitly disable allowRecursive while executing these pre-render hooks.

Performance effect

Ideally, this algorithm only needs to iterate the Dep array twice without having to touch any of the sets. It is a robust performance enhancement, and performs stable even when the order or tracked dependencies changes!

This has a huge boost on all effects, and especially those with a lot of dependencies. The mix benchmark, which represents a real-world scenario, was boosted by almost 40%!

performance-comparison

@basvanmeurs basvanmeurs marked this pull request as draft June 30, 2021 13:07
@basvanmeurs basvanmeurs marked this pull request as ready for review June 30, 2021 13:14
@basvanmeurs
Copy link
Contributor Author

Note: this PR has been tested and verified in our local project which uses advanced reactivity.

@yyx990803 yyx990803 added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. version: minor labels Jul 1, 2021
@yyx990803 yyx990803 force-pushed the 3.2 branch 3 times, most recently from 2046d36 to ba1d97c Compare July 6, 2021 18:35
@yyx990803
Copy link
Member

I rebase 3.2 branch on top of latest master which seems to cause some unintentional conflicts... could you please rebase your branch?

@yyx990803
Copy link
Member

I'll try to review and merge this one after we resolve #2195 so hopefully this should be the last time 😅

@basvanmeurs
Copy link
Contributor Author

basvanmeurs commented Jul 7, 2021

could you please rebase your branch?

Rebase to 3.2 I suppose? Done!

@yyx990803 yyx990803 merged commit a889895 into vuejs:3.2 Jul 7, 2021
@yyx990803
Copy link
Member

Great work, thank you so much @basvanmeurs !

Note I refactored this PR in ab8bc71 mainly to cut down the size increase (+0.29kb -> 0.21kb) since the new additions are non-tree-shakeable.

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. version: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants