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

Optimize flushChanges via WeakMap<Atom, ArraySet<EffectScheduler>> #73

Open
justjake opened this issue Apr 12, 2023 · 1 comment
Open

Comments

@justjake
Copy link

This strategy comes from https://github.com/starbeamjs/starbeam

Currently when we write to an Atom, we recursively search the children to notify EffectScheduler subscribers. The problem with this kind of graph traversal in Javascript is that it's not very cache friendly. We can skip some work using the lastTraversedEpoch which effectively memoizes the traversal for already-notified subscribers. The work skipping helps in the best case, but doesn't reduce time spent in the worst case.

My proposed optimization is that we maintain a const ATOM_SUBSCRIBERS = new WeakMap<Atom, ArraySet<EffectScheduler>> which stores a direct mapping from mounted atoms to effects. If we can correctly maintain this set, the notification algorithm becomes strictly O(subscribers) instead of O(graph):

function flushChanges(atoms: Iterable<_Atom<any>>) {
  const reactors = new Set<EffectScheduler<unknown>>()
  for (const atom of atoms) {
    ATOM_SUBSCRIBERS.get(atom)?.forEach(effect => reactors.add(effect))
  }
  reactors.forEach(effect => r.maybeScheduleEffect())
} 

I haven't spent much time on the maintenance algorithm for ATOM_SUBSCRIBERS, but we should be able to do it in stopCapturingParents or similar by doing an upwards crawl of the parent. I don't recall how Starbeam implements it.

Maybe the maintenance time plus the storage space of any required memoize overwhelm the savings, but I think it could be a profitable optimization strategy.

@ds300
Copy link
Contributor

ds300 commented Apr 13, 2023

It's a nice idea! I had the same idea but I ran into issues trying to implement it back in the early days of signia.

The problem is:

During a transaction the dependency graph may change, and it becomes possible for a change to an atom to need to flow to a reactor that it wasn't connected to before the start of the transaction. Which meant that any time the parents of any computed changed during a transaction we had to traverse it's entire parent hierarchy to find root atoms, and then make sure it's list of connected reactors was up to date, while detaching the reactors of any atoms no longer in the parent hierarchy.

I tried to optimize that by caching the parent atoms at each computed node, but that was slow and buggy and complicated, so I didn't pursue it further.

I'd be interested to find out whether starbeam is both pull-based and has transactions, and if so whether their stuff resolves the issue I ran into somehow.

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

No branches or pull requests

2 participants