fix: improve performance of scheduling effects

[trueadm]

This very simple change has quite a significant impact on our local benchmark. In the case of some of the benchmarks, they're not wrapped in a branch effect, only a render effect. We now wrap them with a branch effect!

[changeset-bot]

🦋 Changeset detected

Latest commit: 9423767

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dummdidumm]

Does it makes sense to investigate avoiding the includes check and possibly even the walking up to the parents logic? I see that two usage sites first set the effect to dirty, then schedule it. Can we use a method in these places that does both, and bails early if we detect that the effect is already dirty? The mark_reactions usage site seems more involved, not sure how to improve it there.

[trueadm]

@dummdidumm I looked into that, but we can't. schedule_effect doesn't visit the same things that mark_reactions visits, one is the reactive graph the other is the effect tree. We propagate up to the effect tree to the root and mark all the branches as maybe dirty to avoid doing too much work later.

I also tried doing a simple queued_root_effects[0] !== effect and because queued_root_effects is generally always empty or has one thing in it (except this benchmark) it didn't make any perf difference. I also tried marking the root as maybe dirty too, but that literally breaks every test as roots are never marked as dirty – and knowing when to mark them as clean isn't straightforward either.

[Rich-Harris]

In the case of some of the benchmarks, they're not wrapped in a branch effect, only a render effect

Are we saying that this only occurs inside benchmarks, and not inside real apps? If so, won't this make real apps slower for the sake of those benchmarks?

[trueadm]

@Rich-Harris It's definitely possible in real world applications that make use of $effect.root too, as there's no branch effect anywhere.

[trueadm]

Okay, I've removed the includes logic and instead opted to wrap the $effect.root with a branch now which also solves the problem nicely for everyone!

[Rich-Harris]

We call branch(() => ...) directly inside the effect root created in _mount — should effect_root just always create a branch? The only other place it's used is inside toStore, where a branch seems harmless.

Though, going deeper: I don't really follow what the if ((flags & BRANCH_EFFECT) !== 0) { inside schedule_effect is for — why do we only bail out early if it's a branch effect? I'm sure there's a good reason but it's unclear from the code — at the very least it could use an explanatory comment. If we weren't doing that then the other changes would presumably be unnecessary

[trueadm]

Yes we only bail out on branches. That's because they're not part of the reactive graph and thus cannot be made dirty from the reactive graph, so we use the MAYBE_DIRTY to flag that we need to visit them otherwise we skip them entirely in both schedule_effect and also process_effects. I'll add a comment.

We call branch(() => ...) directly inside the effect root created in _mount — should effect_root just always create a branch? The only other place it's used is inside toStore, where a branch seems harmless.

I can do that too.

[trueadm]

Right ready for review, I've made all the changes based off feedback.

[trueadm]

I actually feel pretty confident that this PR is a better direction than #13297. Writes are always more expensive than reads, and the less of them we have to do, the better. Ironically branch effects just happen to be a good place for that logic to exist IMO.

[Rich-Harris]

what's the purpose of these changes? they pass on main already, so what are they testing exactly?

[trueadm]

That's because effect_root didn't create a branch before. Because child effects get attached to their parent after their contents run, the flushSync doesn't do anything as the root has no child effects.