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

chore: only call mark_reactions when updating a source #12272

Merged
merged 17 commits into from
Jul 3, 2024
Merged

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Jul 3, 2024

This implements something I've been wanting to get at for a while. Currently, we call mark_reactions in two places — when updating a source, and when updating a derived. The latter doesn't really make sense — we should only mark reactions when sources update.

This fixes that, and allows us to simplify things a bit — there's no more force_schedule. It also allows us to move mark_reactions (and inspect_effects) out of the neverending module that is runtime.js and into sources.js.

It works by using version tracking as the universal mechanism for knowing if a reaction is dirty. We can use current_version for effects, rather than increment_version(), so this doesn't increase the (infinitesimal) danger of an overflow. (I thought I'd be able to use the same trick for deriveds, but it doesn't work for some reason — will continue investigating.)

Performance-wise it's basically neutral, except for kairo_mux_unowned which is slower. I'm not exactly sure why but my hunch is that it's somehow related to the fact that we're pushing reactions further down — it feels weird that we do that there in addition to doing it inside update_reaction. Feels off, will look into it. With the most recent commits, this is significantly faster than main. I'm still a bit confused by the is_unowned stuff inside check_dirtiness, but we can worry about that another time.

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

Copy link

changeset-bot bot commented Jul 3, 2024

⚠️ No Changeset found

Latest commit: 3699d41

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 trueadm merged commit 15873ab into main Jul 3, 2024
9 checks passed
@trueadm trueadm deleted the effect-version branch July 3, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants