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

RFC: A simpler implementation #65

Merged
merged 60 commits into from
Nov 16, 2015
Merged

RFC: A simpler implementation #65

merged 60 commits into from
Nov 16, 2015

Conversation

shashi
Copy link
Member

@shashi shashi commented Aug 29, 2015

This is a rewrite of Reactive to make it more robust

How it works

  • A Node contains the current value and a vector of Actions
type Node{T}
    value::T
    actions::Vector{Action}
    alive::Bool
end

immutable Action
    recipient::WeakRef
    f::Function
end
isrequired(a::Action) = a.recipient.value != nothing && a.recipient.value.alive
  • add_action!(f, node, recipient_node) creates an Action with weakref to recipient_node and function f. send_value!(node, val, timestep) sets val as node's value and calls all the actions with the recipient_node and timestep as the two arguments to f. timestep is a counter that increases with every new message.
  • connect_* functions (e.g. connect_map) take both input and output nodes and add appropriate actions to them (spot the difference with this example in sicp)
  • exported functions like map, foldp, etc create a new output node and connect them to input nodes using the respective connect_* function
  • _messages is a global channel. push! writes (node, new_value, metadata) to the channel.
  • Reactive.run(n) reads n messages off this channel and propagates the changes.
    It throws a ReactiveException with data to debug the crash. a ReactiveException prints a comprehensive crash message, for example:
ERROR: Failed to push!
    1
to node
    Node{Int64}(1, nactions=1)
at timestep 1

Debug info:
    ["meta"=>"kwarg","passed"=>"to push!","comes back"=>"here"]

error message goes here blah blah
 in buggy_function at /home/shashi/.julia/v0.3/Reactive/test/example.jl:10
 in anonymous at /home/shashi/.julia/v0.3/Reactive/src/operators.jl:39
 in send_value! at /home/shashi/.julia/v0.3/Reactive/src/core.jl:66
 in run at /home/shashi/.julia/v0.3/Reactive/src/core.jl:121
 in run at /home/shashi/.julia/v0.3/Reactive/src/core.jl:111
 in include at ./boot.jl:245
 in include_from_node1 at loading.jl:128
 in process_options at ./client.jl:285
 in _start at ./client.jl:354
 in run at /home/shashi/.julia/v0.3/Reactive/src/core.jl:125
 in run at /home/shashi/.julia/v0.3/Reactive/src/core.jl:111
 in include at ./boot.jl:245
 in include_from_node1 at loading.jl:128
 in process_options at ./client.jl:285
 in _start at ./client.jl:354
while loading /home/shashi/.julia/v0.3/Reactive/test/example.jl, in expression starting on line 20
  • no @lift macro - it is too complex as it stands

API changes

This is a breaking change

  • Signal and Input are both typealiases to Node.Input{Type}(val) is now Input(Type, val)
  • lift is now map
  • foldl is now foldp - for fold-over-past
  • merge breaks the guarantee of keeping the update from the earliest
    argument if more than one arguments update in the same timestep - the
    oldest node gets priority (the propagation is depth-first)
  • the main process needs to run Reactive.run() and watch for exceptions. A ReactiveException is raised which captures exceptions, backtraces and more info. exceptions don't get lost into the ether
  • push! takes optional meta argument which shows up in the ReactiveException object if an exception occurs
  • close(node) makes the node refuse further updates
  • added delay, debounce and boundcopy

You can call push! inside a consume this simply queues up the update for the next timestep.

This fixes #54 #62 #61 #24

TODO

cc @SimonDanisch @Keno @stevengj @aviks @amitmurthy

This is a simpler implementation of Reactive.

- A signal Node contains the value and a vector of weak refs of functions that need to be run when the node updates
- `connect_*` functions (e.g. consume_connect) take both input and output
  nodes and add appropriate actions to them
- exported functions like `consume`, `foldp`, etc create new nodes and
  connect them to the input nodes using the respective `connect_*`
function
- `_messages` is a global channel. `push!` writes `(node, new_value, metadata)` to the channel. there is
  _recipients_dict which is a weak key dict of nodes to a list of action functions *that depend on it*
-  `Reactive.run(n)` reads n messages off this channel and propagates the changes.
   It throws a `ReactiveException`  with data to debug the crash.
- no `@lift` macro - it is too complex as it stands

This is a breaking change

- Signal and Input are both typealiases to Node. `Input{Type}(val)` is now `Input(Type, val)`
- `push!` takes optional debug metadata argument which shows up in the `ReactiveException` object if an
  exception occurs
- `merge` breaks the guarantee of keeping the update from the earliest
  argument if more than one arguments update in the same timestep - the
oldest node gets priority (the propagation is depth-first)
- the main process needs to run `Reactive.run()` and watch for exceptions.
- added `delay`, `debounce` and `boundcopy`
- setting `node.alive` to false stops updates from propagating from a node

You can call `push!` inside a `consume` this simply queues up the update for the next timestep.
iter += 1

waiting = true
(node, value, debug_meta) = take!(_messages)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you could address #62 by doing an isready loop and setting the value of all the nodes at once while queuing the actions in a Set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating many inputs at once may change some semantics. E.g. the value of foldp(x -> x+1, 0, map(tuple, signal1, signal2)) will not be the same for the same sequences of input to signal1 and signal2 depending on how fast the runner is able to clear the queue... It also affects sampleon, filterwhen in subtle ways as well (but maybe these two are not as important as foldp)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understand the complexities much better than me. I'll simply add a use-case to #62.

@timholy
Copy link
Member

timholy commented Sep 4, 2015

I'm pretty new to Reactive and haven't yet used it in a serious project, but a big 👍 for this. I'm especially excited to see the removal of all of those heap operations and like the lift->map rename. I'm planning on becoming an adopter.

While you're renaming things, I'll confess that the push!, value pair seems a little strange to me: I guess I kind of expect that push! implies a growing container, but a Node just stores a single value. What about set! and get? Or put! and get/take? But the status quo is fine if it makes more sense to you. Whatever words you choose here, the same ones should apply to widgets in Interactive.jl.

@shashi
Copy link
Member Author

shashi commented Sep 4, 2015

@timholy great to hear! :)

I agree push! was ill-conceived. It seems put! or set! would indeed be better names for this. Both good candidates.

@timholy
Copy link
Member

timholy commented Sep 4, 2015

Would be good to also have the names reflect the "imperative"/"declarative" distinction clearly. set!/put! seems like it should take effect immediately to me, kind of like the implementation of send_value! (which to me implies something more like a remote-ref operation...). Now that I look at it, push! is not such a bad name for what it does now; however, one slightly confusing thing is that you're effectively pushing onto _messages rather than n---but it's complicated because it will eventually set! n. set!_queued(n, x) or queue_set!(n, x)??

Wow, naming is hard.

@timholy
Copy link
Member

timholy commented Sep 4, 2015

Or maybe @queue set!(n, x). I like that better than my other two suggestions.

@shashi
Copy link
Member Author

shashi commented Sep 4, 2015

why a @queue macro when the job can be done by a function..? I think queue_set! is equally good, and better than send_value! 👍

@timholy
Copy link
Member

timholy commented Sep 4, 2015

I was thinking that @queue set!(n, x) would just rewrite the code as queue_set!(n, x), but somehow I thought the macro syntax (a little remiscent of @async) made it clearer that you are queuing a set! operation.

@timholy
Copy link
Member

timholy commented Sep 4, 2015

(Almost like queue(set!(n, x)), which of course wouldn't work.)

@shashi
Copy link
Member Author

shashi commented Sep 4, 2015

Ah, I see the similarity with @async, but I think queue_set! will do here. It's not part of the public API anyway. One of the changes here (I just added to the list) is foldl being renamed to foldp - I'm more concerned how that will go...

@shashi
Copy link
Member Author

shashi commented Nov 14, 2015

We seem to have correct implementations of everything we need now.

Couple of questions to answer to get this merged:

  1. Do we need bottom-up GC of the graph like we had before dfabab7 ? @SimonDanisch doesn't seem to like sprinkling preserves very much. @timholy what do you think?
  2. The names Signal, Node and Input all point to the same thing... This one is a trifling matter, but it is slightly important that we bikeshed this. We ideally should to keep just one name, the best candidate seems to be Signal. We should deprecate Input for Signal if that makes sense to everyone (I feel the removal of Input will help understanding). Node is a good name internally (they are like wires which are connected to become or hold a signal) Signal is a good name in the public API. My odd preference here is to keep the type's name as Node but export Signal (typealias of Node). Here is the code. I will reluctantly agree to rename Node to Signal internally if that's what people want.

@SimonDanisch
Copy link
Member

1.)
Can you explain a little more? What's possible and what isn't right now? I think I'd be fine with sprinkling preserves, as long as they follow very understandable rules! If at some point the general rule of thumb would be: "rather put a preserve around your signal to be on the safe side", we'd done something wrong ;)
2.)
I agree with your preferences... Signal makes most sense to the user, while Node is a good name internally!

@shashi
Copy link
Member Author

shashi commented Nov 14, 2015

  1. Right now if a signal a is used to create several other signals, then either they all stop updating when they all go out of scope or all of them sit around and continue to sit around and update unless you close them explicitly. What's possible is: if a is used to create several signals, so as to make a graph of nodes, then any node whose children have gone out of scope (or a leaf node which has gone out of scope) will automatically stop updating after a gc pause. This means you will require preserve in places where you are doing side-effects e.g. preserve(map(println, a)) instead of just map(println, a) so that the map node will continue to exist as long as a does. I don't know if any other reactive programming systems do this... I couldn't find out much by googling.
  2. I'm relieved one more person agrees! :)

@timholy
Copy link
Member

timholy commented Nov 14, 2015

We seem to have correct implementations of everything we need now.

Congratulations!

Do we need bottom-up GC of the graph like we had before dfabab7

I am torn on this one. On balance, I suspect that it would be better that way.

But I do have a question: suppose (like fps) we return a node/signal:

a = foo(7)

but internally in the course of creating a, we also need to create a "derived" node/signal b that we don't return to the user. We could, of course, call preserve(b), but then I think that will keep a from being gced?

Node/Signal/Input

Yes, definitely good to reduce the number of redundant names. You know about @deprecate_binding? (Used for deprecating types or other names.)

I agree with the preference Node internally. Externally, I have a slight preference for keeping it Node (rather than Signal) just for reasons of consistency (to facilitate blurring the lines between "user" and "developer" 😉). But I will be fine with whatever you choose.

@shashi
Copy link
Member Author

shashi commented Nov 15, 2015

but internally in the course of creating a, we also need to create a "derived" node/signal b that we don't return to the user. We could, of course, call preserve(b), but then I think that will keep a from being gced?

I have another implementation of preserve which will allow b and a to be gc-ed together.

You know about @deprecate_binding? (Used for deprecating types or other names.)

I didn't know about it, will use it. Thanks!

Node seems to be too general / meaningless a name to export...

@shashi
Copy link
Member Author

shashi commented Nov 15, 2015

Here's the preserve we need.

SimonDanisch added a commit to JuliaGL/GLVisualize.jl that referenced this pull request Nov 15, 2015
@timholy
Copy link
Member

timholy commented Nov 15, 2015

I agree that Node is too general to export. That's a good reason for exporting Signal.

Conflicts:
	.travis.yml
	REQUIRE
	src/Reactive.jl
	src/timing.jl
	test/basics.jl
	test/flatten.jl
	test/macro.jl
	test/trylift.jl
it prints Reactive.Node instead of
Reactive.Signal

This reverts commit acffeb1.
shashi added a commit that referenced this pull request Nov 16, 2015
RFC: A simpler implementation
@shashi shashi merged commit deb3f00 into master Nov 16, 2015
@SimonDanisch
Copy link
Member

Awesome! Thanks for the great work :)

@timholy
Copy link
Member

timholy commented Nov 16, 2015

Yes indeed!

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.

Feature Request: Stopping a signal
6 participants