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

Implement accumL with newPulse #260

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Implement accumL with newPulse #260

merged 1 commit into from
Aug 11, 2022

Conversation

ocharles
Copy link
Collaborator

Currently accumL uses mapL to read the previous state whenever the state update Pulse fires. This is problematic, because when the previous state is read in mapL (and transformed into a (a -> a) -> a function), this is cached into the Latch. This means that when the state update Pulse fires, we'll update one latch to the new state, while also retaining the previous state. This commit redefines accumL as a call to newLatch (as before) and creates a new Pulse with newPulse whose evaluation function reads the Pulse containing the a -> a endmorphism, and also reads the current state from its latch. With this implementation, no historic values are leaked by accumL.

@mitchellwrosen
Copy link
Collaborator

I'm not 100% clear on what's going on but I trust that this makes things better. At the very least I can see how this allocates one fewer latch nodes, so that seems good.

This means that when the state update Pulse fires, we'll update one latch to the new state, while also retaining the previous state.

A few questions - mostly permutations of the same question, really - where are we retaining the previous state? Do you mean in memory somewhere, due to laziness or something? (Like, does the (\x f -> f x) thing matter here, or is this transformation something we should do to all applyP (mapL ...) things?) I'd have naively guessed that the latch is updated to the latest state via its latch write node, which is a child of pulse we make, so I'm not seeing where the historical values are leaked. Is it a space leak that grows and grows, or just a single-step leak?

mitchellwrosen
mitchellwrosen approved these changes Jun 14, 2022
@ocharles
Copy link
Collaborator Author

ocharles commented Jun 15, 2022

It's a tricky one to explain, but lemme try again 😄

First, the setup:

  • accumL creates the Latch x and the Pulse p2. x initially has value a.
  • p2 is created by calling applyP with the Latch mapL (\a f -> f a) x. I'll call this latch m. It also takes p1.
  • x is set to update itself with the latest result of p2.

Now, let's look at what happens when p1 fires:

  • p1 fires, giving us the function g.
  • p2 depends on p1, so we evaluate p2.
  • p2's evaluation needs to read latch m, which gives us a (a -> a) -> a, and we read p1s currently value to get an a -> a. We can then combine these to get a new a.
  • When we read m, we'll evaluate that latch and cache the result. To do this, we need to evaluate a mapL Latch, which we do by reading the latch that m depends on - x - and transform this into \f -> f a. This function closure then gets written back into m (as mapL is a cachedLatch).
  • Next, we read p1 to get the function g and apply this with \f -> f x ((\f -> f x) g).
  • We've now evaluated p2 and produced a'
  • As we have updateOn p2 for x, we'll update the Latch x to store a' at the end of evaluation.

The state after network evaluation is now:

  • x has value a', the new state.
  • m has value \f -> f a, which is a function closure that has a reference back to the old state.

Does that help?

Is it a space leak that grows and grows, or just a single-step leak?

It's the first, but single-step is a bit ambiguous. It's a single step leak on accumL's history. This means that you might hold onto this historic value for many steps, if the input to accumL isn't firing frequently. Essentially accumL always holds onto its current state and it's previous state.

Do you mean in memory somewhere, due to laziness or something?

Also just to clarify, this is not a problem with laziness.

I'd have naively guessed that the latch is updated to the latest state via its latch write node.

This is correct, but only for the latch x - the problem is the intermediate cachedLatch produced by mapL.

I should also note that this is part of the problem described by #259.

@HeinrichApfelmus
Copy link
Owner

Thanks for catching this and sorry for taking so long to take a look at this -- work. 🤪

I agree with you, @ocharles

  • the 'mapL' creates an additional Latch, whose value is going to be cached
  • moreover, this value has a function type, and those can often contain more data than necessary

I approve, feel free to merge. 🙏🏻

@ocharles
Copy link
Collaborator Author

@HeinrichApfelmus You need a job like mine where reactive-banana is part of your work 😉 Thanks for the review, no worries about taking your time - I'll get this merged now!

@mitchellwrosen
Copy link
Collaborator

Whoa, who's hiring banana engineers?!

@ocharles
Copy link
Collaborator Author

CircuitHub 😎 But not hiring at the moment unfortunately 😞

@ocharles ocharles merged commit 06cbb02 into master Aug 11, 2022
@ocharles ocharles deleted the accumL-newPulse branch August 11, 2022 07:32
@HeinrichApfelmus
Copy link
Owner

CircuitHub 😎 But not hiring at the moment unfortunately 😞

If they feel the need to hire again — you know where to find us. 😉

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.

3 participants