8000 Breadth First Subscriber Resolution by btakita · Pull Request #3219 · sveltejs/svelte · GitHub
[go: up one dir, main page]

Skip to content

Breadth First Subscriber Resolution #3219

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

Merged
merged 5 commits into from
Jul 26, 2019
Merged

Conversation

btakita
Copy link
Contributor
@btakita btakita commented Jul 11, 2019

fixes #3191

All invalidators of subscribers are run on a derived store when invalidated.

See #2955

@btakita btakita changed the title fixes https://github.com/sveltejs/svelte/issues/3191 Derived store reruns subscribers if it's value has not changed when synced. Jul 11, 2019
@btakita
Copy link
Contributor Author
btakita commented Jul 11, 2019

This PR is way too slow. My app's page maxes out the CPU & does not load.

I'll leave it here for reference, however it looks like the naive approach is no bueno.

@btakita
Copy link
Contributor Author
btakita commented Jul 11, 2019

The latest commit performs significantly better than the first. This PR should be ready to go.

I'll be testing it using https://github.com/btakita/svelte/tree/gh-3191-hotfix

@mrkishi
Copy link
Member
mrkishi commented Jul 11, 2019

It looks like this has no diamond dependencies consolidation for asynchronous derived stores, right?

But is it even a big deal? Should diamond dependencies cause one update even for async deriveds?

@btakita
Copy link
Contributor Author
btakita commented Jul 11, 2019

I've always had the understanding that the observers need to resolve their own diamond dependency state transitions by using guard conditionals.

Breadth first subscriber resolution effectively enables automatic diamond dependency resolution for synchronous derived stores. Re: asynchronous stores, it would be nice to have diamond dependency resolution. Perhaps the start function, passed to derived, can receive an additional argument, which would be the invalidate function?

derived([a, b], async ([$a, $b], set, invalidate) => {
  invalidate()
  const [$a_, $b_] = await Promise.all([a_fn($a), b_fn($b)])
  set($a_ + $b_)
})

Calling the invalidate function is optional, due to the bitwise logic (|=) of the pending flags.

@btakita
Copy link
Contributor Author
btakita commented Jul 11, 2019

One could also do multi-step async state-machine flows with the invalidate function.

derived([a, b], async ([$a, $b], set, invalidate) => {
  invalidate()
  const $a_ = await a_fn($a)
  set({ step: 1, $a_ })
  invalidate()
  const $b_ = await b_fn($b)
  set({ step: 2, $a_, $b_ }
})

@btakita btakita changed the title Derived store reruns subscribers if it's value has not changed when synced. Breadth First Subscriber Resolution Jul 11, 2019
btakita added 4 commits July 23, 2019 17:39
Derived store reruns subscribers if it's value has not changed when synced.

All invalidators of subscribers are run on a derived store when invalidated.

See sveltejs#2955
Optimize performance by using subscriber_queue.
@Rich-Harris
Copy link
Member

Thank you. Took me a long time to understand what was going on here, so I'm going to leave some notes to preserve my current state of enlightenment:

The root issue here was that invalidating a derived store would cause all of its dependents, direct or otherwise, to immediately invalidate as well, on the expectation that the subscriber would run (and revalidate) immediately after that. But that's not true, since an intermediate deriver might return the same value as before in which case its dependents are never revalidated.

This depth-first approach to invalidation also meant that invalidation was happening more frequently than necessary — if B depends on A and C depends on B, then changing A will invalidate B and C, then updating B will cause C to be invalidated again, harmlessly but pointlessly.

Switching to a breadth-first approach fixes that; we only invalidate derived stores whose inputs have in fact changed.

(Thanks for bearing with me during this episode of 'Rich learns algorithms' — I could have saved myself some mental effort if I'd just seen @mrkishi's comment sooner.)

It does introduce a theoretical downside, in that this aspect of derived's behaviour is closely tied to the implementation of writable, making it harder to use with arbitrary objects that fulfil the store contract. But I think you can argue that that's already the case, and that if you're bringing in writable alternatives they probably come with their own derived alternatives.

I made a small change — we can now return the readable store directly from derived, and I got rid of the .shift() calls in favour of resetting the subscriber_queue length to zero, since that performs better. (In microbenchmarks arr = [] seems to outperform arr.length = 0, but I suspect that's because it's just deferring the work until the next GC sweep.)

I'm not certain if we want to handle the async case — it feels like adding a fair bit of complexity for somewhat incremental gains. At that point it might be better to reach for something more powerful than Svelte's built-in stuff. But we can discuss that on #3233.

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.

Derived stores with multiple dependencies don't always update.
3 participants
0