Skip to content

Implement accumL with newPulse - #260

Merged
ocharles merged 1 commit into
masterfrom
accumL-newPulse
Aug 11, 2022
Merged

Implement accumL with newPulse#260
ocharles merged 1 commit into
masterfrom
accumL-newPulse

Conversation

@ocharles

Copy link
Copy Markdown
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
Copy Markdown
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

ocharles commented Jun 15, 2022

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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. 🙏🏻

@HeinrichApfelmus
HeinrichApfelmus self-requested a review August 10, 2022 18:44
@ocharles

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

Whoa, who's hiring banana engineers?!

@ocharles

Copy link
Copy Markdown
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
Copy Markdown
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