chore: simplify process_effects

[Rich-Harris]

Follow-up to #13282 (review), alternative to #13282. This adds a new EFFECT_HAS_DIRTY_CHILDREN flag on effects which allows us to make schedule_effect and process_effects more self-explanatory.

Before, schedule_effect would set the MAYBE_DIRTY flag on branches as a proxy for 'this branch has dirty children'. But that's imperfect — it means that we only bail out when we hit a branch, and it means that branches are regarded as potentially dirty even though that's impossible (they are nodes in the effect tree, but unlike other effects they themselves do not update).

Using MAYBE_DIRTY as a proxy value also required some logic inside process_effects that candidly I am not clever enough to comprehend. The code in this PR feels a good deal simpler, at least to me.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

⚠️ No Changeset found

Latest commit: c297b59

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[trueadm]

Did you run the bench on this to confirm the wins?

[Rich-Harris]

I did, but more to confirm there were no regressions. It was basically noise as far as I could tell — by all means take a closer look on your machine if you want to be confident in the change

[trueadm]

Do we still need the branch effect in _mount?

[Rich-Harris]

Tests fail without it. I guess the root component needs to be in a branch, because otherwise things like this will yield the wrong result

svelte/packages/svelte/src/internal/client/reactivity/effects.js

Line 191 in 09527fd

(active_effect.f & RENDER_EFFECT) !== 0 &&

[trueadm]

Okay so after benchmarking this against #13282 and also just the simple check before of !queued_root_effects.includes(effect) and this PR is consistently slower by 300ms on our internal benchmark. Digging into into it, we now spend far longer inside process_effects than before – almost 200ms more! My hunch is because we're now doing far more work:

if (has_dirty_children) current_effect.f ^= EFFECT_HAS_DIRTY_CHILDREN;

However, there are probably other bigger changes to process_effects that make up this change too.

This line takes 81.2ms for the entire benchmark as is by far the most expensive line in this function.

[trueadm]

This line is also super expensive, it's now 12ms, before the:

set_signal_status(effect, MAYBE_DIRTY);

was only 1.9ms

[trueadm]

I love the design changes this PR brings, but it seems that mutating every signal as we traverse up is just really ineffective for performance. In fact it brings performance regressions over what we currently have on main in regards to benchmarks. We also seem to spend far more time in process_effects than before.

So I think we need rethink this implementation. Writes are far more expensive than reads, and I think this is reflected in #13282.