Node Creation Order Redesign #129

JobJob · 2017-03-24T07:15:11Z

Finally got around to cleaning this branch up.

This PR:

Gets signals updating in ~~the~~ a correct (topological) order (the order Signals are created), which fixes bugs in signals updating on certain signal graphs. See the last three test sets here for examples of where the current (breadth-first) and pre Use an action queue/set to ensure actions are performed in the correct order and aren't repeated #113 (depth-first) designs had bugs.
Makes async optional through Reactive.run_async(false) - though it's not well tested, so async is on by default.
Runs faster than current master on @SimonDanisch's benchmark - included in https://github.com/JobJob/Reactive.jl/blob/action_queues_redesign/benchmark/ReactBench.jl - and slightly (though not much) faster in synchronous mode. Is now around 2.5x slower than the non-reactive version on my machine (previous master was about 4x slower).
Adds a PkgBenchmark - based off Shashi's speedups_maybe branch.
Fixes a bug in error reporting.
Adds a small amount of docs about the design and the garbage collection/preserve mechanism.
adds bound_srcs, bound_dests see OP of BindedTo #119
Adds debounce - which behaves as per previous throttle
Fixes throttle issue in throttle signal not updated #123, but note this changes the behaviour of throttle, use debounce if you want the previous behaviour.

Downsides:

~~Can't push! to non input nodes - those created with Signal(...)~~ you can now
~~flatten~~ bind implementation is a little bit more complicated, though may be more correct in some cases.
May have bugs that aren't tested for in the tests, since the design is a little different.
~~Haven't checked if it plays nice with Garbage Collection~~

Todo:

Fix interaction with Garbage Collection
Clean up some references in comments to action_queue
Add NEWS.md entry about throttle.
Fix debug_memory code
Make throttle tests more robust
Fix Travis test failures

shashi · 2017-03-24T09:05:27Z

Exciting!

SimonDanisch · 2017-03-24T09:30:51Z

Nice! :)
Can we avoid downside 1?! I'm not heavily relying on it, but I'm using it here and there ;)

JobJob · 2017-03-24T10:12:33Z

Can we avoid downside 1?! I'm not heavily relying on it, but I'm using it here and there ;)

Will be hard to do it cleanly now I think, will be so much better if you can live without it ;) I've mostly used up the time budget I allocated to cleaning this up 😬

When I was thinking about this issue, I figured that if needed, people could just bind! a new Signal to the one they previously wanted to push! to and use that instead, it would then function like the original Signal, but you could also push! to that new Signal when you wanted to.

JobJob · 2017-03-24T10:35:15Z

Is that ok? I'll give it a little more thought, it might be doable, but I'd much prefer not to 😄 I am curious about the situations you want to do it, I guess I'd be more inclined to looking into it if it makes doing some relatively common things much easier.

SimonDanisch · 2017-03-24T11:07:58Z

Okay fair enough ;) My use case is that I have my API designed also for people who don't want to use Reactive:

visualization = visualize(..., color = user_signal) # user_signal might also be a default, non Signal node created by me)
set_arg!(visualization, :color, newvalue) # should push into the signal, regardless of what got there

Ah well, I'll need a redesign anyways...

JobJob · 2017-03-24T14:09:18Z

I thought I'd quickly whip up an alternative for you @SimonDanisch but I found a bug in bind!, the fixes are now pushed. I had to disable two-way bind.

I've also pushed the branch (action_queues_redesign) to this (JuliaGizmos) repo, to make it easier to try out.

Anyway here's one way you could do what (I think) you want using bind!:

using Reactive, Interact, Colors

immutable Viz
    color::Signal
end
r,g,b = Signal.([0.7, 0.1, 0.3]) #create r,g and b signals
default_color_sig = 
    map((rv,gv,bv)->RGB(rv,gv,bv), r,g,b; name="default_color_sig") #default sig is a map

visualization(user_signal = default_color_sig) = begin
    color_sig = Signal(colorant"cyan"; name="pushable_color_sig") #can also be pushed to
    bind!(color_sig, user_signal) #if user_signal is updated, update the color_sig
    Viz(color_sig)
end

set_arg!(viz, sym, val) = push!(getfield(viz, sym), val) #will work with a regular value
the_viz = visualization()
display(the_viz.color);

then you can update it with e.g.

set_arg!(the_viz, :color, colorant"pink")

or

push!(g, 0.9)

or

push!(the_viz.color, colorant"yellow")

JobJob · 2017-03-25T12:29:13Z

I think I can actually allow pushing to non-input nodes in a relatively clean way. Will confirm shortly.

JobJob · 2017-03-25T14:28:49Z

Ok yes, push! works again to non-input nodes, and I managed to restore the previous two way bind behaviour, so all looking pretty good. Will check the gc stuff in the next couple of days.

timholy

This isn't complete, but just a few things I noticed while checking the code.

timholy · 2017-04-15T21:54:27Z

doc/action queue design.md

+
+#### Filter
+
+Filter works by setting the filter node's active field to false when the filter condition is false. Downstream nodes (nodes later in the current `action_queue`) check if at least one of their parents has been active, if none of them have been active then the node will not run it's action, thus propagating the filter correctly.


it's -> its

timholy · 2017-04-15T21:55:00Z

doc/action queue design.md

+
+#### Flatten
+
+Implementation is a bit fiddly. See comments in the code for how the graph is rewired.


Maybe one sentence on the overall purpose of flatten? flatten is not "externally" documented elsewhere, I think.

Could definitely flesh that doc out a bit, but low priority for now. The docstring for flatten has a bit of info. Please do let me know if you have questions about the implementation though.

timholy · 2017-04-16T10:38:41Z

src/core.jl

+    isempty(maybe_root_node.roots) ? [maybe_root_node] : maybe_root_node.roots
+
+function getroots(parents)
+    roots = Dict{Signal, Bool}()


Why not use a Set?

I don't know why I did that, I like Sets, maybe I'll remember a reason...

timholy · 2017-04-16T10:53:30Z

src/core.jl

-            warn("closing a non-leaf node is not a good idea")
-        empty!(n.actions)
+"""
+remove the action associated with the node from all of it's roots' action_queues


timholy · 2017-04-16T11:02:03Z

src/operators.jl

-                    prev_timestep = timestep
-                end
-            end
+    let


Do you still need the let? (Here and elsewhere.)

I don't think so. Didn't want to mess with closures around the action functions where locals are defined, but I think you're right that they'd probably still work without the lets

timholy · 2017-04-16T11:12:13Z

src/operators.jl

+                #mostly correct but prob not 100%
+                queue = action_queues[root]
+                for action in subtree_actions
+                    deleteat!(queue, findin(queue, 6))


What's the 6 about?

lol I have no idea! Need to work that out, how does this even work!?

I think it's supposed to be action. I guess it worked but would have caused some things to be re-run twice since in some cases since they weren't removed properly.

timholy · 2017-04-16T11:19:51Z

src/operators.jl

+created by calling `map(f, dest)`, `merge(dest, s1)`, `filter(f, dest)`, etc.
+before `bind!(src, dest)`) may be updated more than once when `src` updates. To
+ensure signals update just once per `push!`, call `bind!` before adding signals
+dependent on `dest`.


Was this also an issue with the former implementation?

I don't think so, this was a change late in the day, I should add it to the downsides on the PR notes once I've checked it.

Note this is only a problem if someone does the thing that the little note says not to do, and has a downstream op that has some side-effect that they don't want running more than once per-push, or perhaps some expensive op that would run twice and thus impact performance.

timholy · 2017-04-16T11:21:11Z

src/operators.jl

-            end
+Note that signals dependent on `dest` added before calling `bind!` (e.g. those
+created by calling `map(f, dest)`, `merge(dest, s1)`, `filter(f, dest)`, etc.
+before `bind!(src, dest)`) may be updated more than once when `src` updates. To


Check argument order to bind!

JobJob · 2017-04-19T17:18:42Z

After spending some time with the code in this branch again, trying to fix the bind! issue, I've come to the view that I don't like the design. It's too complicated, too much pain, not enough gain. I have an idea of how to simplify it. Will flesh it out tomorrow, I hope I can at least salvage some of it. Changed to WIP in the interim.

JobJob · 2017-04-21T15:57:21Z

Simpler design is coming along well so far. Will update over the weekend.

JobJob · 2017-04-23T15:30:54Z

Everything's looking pretty good I think on the simpler design. Tests are passing, managed to restore the old bind! behaviour, I think it's about the same speed as my earlier more complicated version (removed some WeakRefs temporarily, so not 100%), and there's less code. Will do some cleanup and push here tomorrow.

JobJob · 2017-04-24T15:47:53Z

Pushed my latest, its running reasonably fast, about the same as the earlier version, possibly slightly faster, but its using fewer WeakRefs which may affect things slightly, since I haven't fixed up how it interacts with Garbage Collection yet. Will do that tomorrow.

There was talk about changing how Reactive behaves with Garbage collection, if someone has some ideas, now would be a good time, otherwise I'll just restore the previous behaviour.

There's a new, still minimal, design doc here: https://github.com/JuliaGizmos/Reactive.jl/blob/action_queues_redesign/doc/Design%20Overview.md

JobJob · 2017-04-25T04:46:15Z

I think this is pretty much ready now

shashi · 2017-04-25T06:01:45Z

doc/Design Overview.md

+
+#### Filter
+
+Filter works by setting the filter node's active field to false when the filter condition is false. Downstream/descendent nodes check if at least one of their parents has been active, if none of them have been active then the node will not run its action, thus propagating the filter correctly.


shashi · 2017-04-25T06:03:40Z

doc/Design Overview.md

+
+Each Signal also has a field `active` which stores whether or not the node was active (had its actions run) in the current push. A Signal will be set to active if it is `push!`ed to, or if any of its parent `Signal`s were active.
+
+On processing each `push!`, we run through `nodes` and execute the actions of each node if any of its parents were active.


I like the simplicity of this design very much. The global channel makes sure that two push!es don't overlap, so this seems correct.

shashi · 2017-04-25T06:05:35Z

doc/dev notes.md

+1. Each signal has a field `preservers`, which is a `Dict{Signal, Int}`, which basically stores the number of times `preserve(x)` has been called on each of its child nodes `x`
+1. Crucially, this Dict holds an active reference to `x` which stops it from getting GC'd
+1. `unpreserve(x)` reduces the count of `preservers[x]` in all of x's parents, and if the count goes to 0, deletes the entry for (reference to) `x` in the `preservers` Dict thus freeing x for garbage collection.
+1. Both `preserve` and `unpreserve` are also called recursively on all parents/ancestors of `x`, this means that all ancestors of x in the signal graph will be preserved, until their parents are GC'd or `unpreserve` is called the same number of times as `preserve` was called on them, or any of their descendants.


Great to have this explanation! 💯

shashi · 2017-04-25T06:09:53Z

src/core.jl

-            nodes[n] = nothing
+        function (::Type{Signal{T}}){T}(v, parents, actions, pres, name)
+            id = length(nodes) + 1
+            n=new{T}(id, v, parents, false, Action[], pres, name, backtrace())


Unrelated, but just wondering if there's any serious problems with storing the backtrace anyway even in non-debug mode. This could potentially allow better error messages.

That seems like a really good idea!

shashi · 2017-04-25T06:13:00Z

src/core.jl

 immutable Action
    recipient::WeakRef
    f::Function
 end

+const nodes = WeakRef[] #stores the nodes in order of creation (which is a topological order for execution of the nodes' actions)
+const edges = Vector{Int}[] #parents to children, useful for plotting graphs


Nice. Should be easy to output graphviz "dot" strings from this.

Yeah, also check this:

using Reactive, LightGraphs, Plots, PlotRecipes pyplot() s = Signal(1; name="sig 1") m2 = map(x->2x, s; name="x2") m3 = map(x->3x, s; name="x3") function LightGraphs.DiGraph{T <: Integer}(edgelists::Vector{Vector{T}}) g = DiGraph(length(edgelists)) for (v1, edgelist) in enumerate(edgelists) foreach(v2->add_edge!(g, v1, v2), edgelist) end g end g = DiGraph(Reactive.edges) graphplot(g, arrow = arrow(:simple, :head, 1.5, 1.0) , names=map(nr->nr.value.name, Reactive.nodes), linewidth=5)

shashi · 2017-04-25T06:14:17Z

src/core.jl

-function add_action!(f, node, recipient)
-    a = Action(WeakRef(recipient), f)
+function add_action!(f, node)
+    a = Action(WeakRef(node), f)


Seems weird that the recipient is actually the sender. Why not just store a = f?

Yeah, I was planning to do that. action.recipient isn't used anymore

Removed Actions, just using Functions now

shashi · 2017-04-25T06:17:09Z

src/core.jl

-cleanup_actions(node::Signal) =
-    node.actions = filter(isrequired, node.actions)
-
+send_value!(node::Signal, x) = (node.value = x)


maybe this shouldn't be a function then. send_value! is a misleading name. Tim Holy pointed this out before, but I wanted to implement this differently using channels. It doesn't make sense anymore.

set_value! could be useful to isolate the state change. I imagine it would get inlined everywhere and not affect performance, but I'll check.

Yes, just changing the name to set_value would be an improvement I think.

set_value or set_value!?

shashi · 2017-04-25T06:26:33Z

src/core.jl

    println(io)
    println(io, "to node")
    print(io, "    ")
-    show(io, node)
+    show(io, pushnode)
    println(io)
    println(io)
    println(io, "error at node: $error_node")


Maybe the backtrace can be used here (unrelated to this PR).

shashi · 2017-04-25T06:28:00Z

src/operators.jl

-            end
-        end
+    add_action!(output) do
+        output.value = f(map(value, inputs)...)


Loving the simplification here!

shashi · 2017-04-25T06:45:58Z

src/operators.jl

-        push!(output, value(input))
+    add_action!(output) do
+        # only push when input is active (avoids it pushing to itself endlessly)
+        input.active && push!(output, value(input))


I think this will be incorrect in the synchronous case. Probably good to check the mode and throw a warning here before the push... (unrelated to this PR)

You're right. I haven't given much thought to what will break in synchronous case. Will think about it.

shashi · 2017-04-25T08:43:06Z

test/flatten.jl

+        # a signal that is the flatten parent sigsig's (c's) current value ("a" here)
+        # then when the sigsig gets pushed another value, you want the map to
+        # still update on changes to a, even after the map is "rewired" when a
+        # new value is pushed to c.


shashi · 2017-04-25T08:49:06Z

test/node_order.jl

+    push!(sx, 3)
+    Reactive.run_till_now()
+    @fact value.([sx, s1x1, s1x2, s2x, s3x]) --> [3, 3, 3, 6, 9]
+end


Thanks for the review!

JobJob · 2017-04-25T14:45:52Z

Tests are failing on Travis, I'm guessing GC related, they work on my local. Will check it out tomorrow.

JobJob · 2017-04-27T11:21:38Z

While I was looking at GC issues, I noticed pushing to non-input nodes wasn't working, and as a result bind! for non-input nodes wasn't either. So since your review @shashi I had to make some changes to bind (it's now simpler), throttle, debounce, delay, fpswhen and a minor change to flatten (slightly simplified). Some of those operations now use a foreach node to run their action. See https://github.com/JobJob/Reactive.jl/blob/action_queues_redesign/doc/dev%20notes.md for some details. Added tests for pushing to non-input nodes, and they're all passing now.

I also made some changes to the way GC'd nodes are removed from nodes it now only happens on the main thread in the push/queue running task (does GC run in a separate thread in 0.5/0.6 or just a task/green thread?). Earlier I was modifying nodes in the finalizer and it was messing with iteration over nodes on the push running task. This seems to have eliminated GC related errors.

It's running reasonably fast. Maybe you wizards can make it run even faster - varies between 2.2x to 2.8x slower than non-reactive function calls on Simon's benchmark on my local. Before the changes to make it do the right thing with GC, it was closer to 2x slower.

Ok now I really think this branch is almost ready to go - just need to squash the commits and clean up the commit messages. I guess I'll make it into 2 commits? One for the previous design and one for the new one?

Anyway, I think it's simpler, faster, and more correct, so, yeah! 😄

shashi · 2017-05-02T09:27:28Z

This is looking very good. The only minor comment I have is about throttle changing behavior since previous versions. Any ideas about how to possibly deprecate neatly? I think @timholy and @SimonDanisch are the only two users of throttle though. They've now been warned anyway ;)

It would be nice to bring this down to <= 10 meaningful commits. I understand you're short on spare time, so I'm fine with you doing whatever is the easiest option for you. Thanks for taking this up and putting in so much amazing effort! 🎉

JobJob · 2017-05-02T11:09:25Z

I think @timholy and @SimonDanisch are the only two users of throttle though

Ha. I suppose we could add a warning like this when throttle is called: "Throttle's behaviour has changed, see ?throttle for more details, for the old behaviour use debounce. To disable this message call Reactive.no_throttle_warn()". Then in x number of point releases, remove the warning in throttle, and show a message when Reactive.no_throttle_warn is called, saying, e.g. "This function is no longer needed". (edit: or maybe could use an environment variable e.g. ENV["REACTIVE_THROTTLE_WARN"], and when we remove the warning, just test for its presence in Reactive.__init__ and say it's no longer needed). What do you guys think?

It would be nice to bring this down to <= 10 meaningful commits. I understand you're short on spare time, so I'm fine with you doing whatever is the easiest option for you.

Yep I'll squash them to some smaller number in the next few days, see how I go.

Thanks for taking this up and putting in so much amazing effort! 🎉

Thanks to you and @timholy for the reviews on this stupidly large PR 😄 I'm really glad its worked out 🎉

@SimonDanisch

Gets signals updating in a correct (topological) order (the order Signals are created), which fixes bugs in signals updating on certain signal graphs. See the last three testsets in node_order.jl for examples of where the current (breadth-first) and pre JuliaGizmos#113 (depth-first) designs had bugs. Makes async optional through Reactive.run_async(false) - though it's not well tested, so async is on by default. Adds some docs about the design and the garbage collection/preserve mechanism in doc/Design Overview.md and doc/dev notes.md Improves performance, runs faster than current master on @SimonDanisch's benchmark - included in benchmark/ReactBench.jl - and slightly (though not much) faster in synchronous mode. Is now around 2.5x slower than the non-reactive version on my machine (previous master was about 4x slower). Adds a PkgBenchmark - based off Shashi's speedups_maybe branch. Fixes a bug in error reporting. adds bound_srcs, bound_dests see OP of JuliaGizmos#119 Adds debounce - which behaves as per previous throttle Fixes throttle issue in JuliaGizmos#123, but note this changes the behaviour of throttle, use debounce if you want the previous behaviour.

JobJob · 2017-05-07T09:55:34Z

Ok done. With all the merge commits in this (old) branch, rebase was making life difficult, so I just started afresh and used git read-tree ..., so just have the one commit. Too hard/no time to do anymore. I have the entire old branch in my fork here for safe-keeping.

Anyway, I think it's cowabunga time! 😄

shashi · 2017-05-07T10:15:23Z

Works for me, merging! A bit sad that this won't help you much in the dark-green-squares-in-your-github-profile game though. 😜

JobJob · 2017-05-07T11:45:16Z

Haha, just one light green square - very rough - not to mention still being 5th in the number of commits leaderboard for this repo.

Regardless - party time, excellent! 🎉 🎉 🎉

timholy · 2017-05-07T11:48:54Z

Heroes are not measured in numbers of commits! Thanks, @JobJob!

timholy · 2017-05-08T17:55:06Z

I'm very pleased to report that for both GtkReactive and a just-submitted rewrite of ImageView, this huge transition was absolutely seamless---only one line of test code will need changing (an explicit test of how signals print in GtkReactive). Importantly, all the "real" functionality Just Worked. Really amazing work, @JobJob!

JobJob · 2017-05-09T07:44:13Z

Great to hear!

timholy mentioned this pull request Apr 15, 2017

World age #131

Closed

timholy reviewed Apr 16, 2017

View reviewed changes

JobJob changed the title ~~Action queues redesign~~ WIP: Action queues redesign Apr 19, 2017

JobJob changed the title ~~WIP: Action queues redesign~~ Node Creation Order Redesign Apr 25, 2017

shashi reviewed Apr 25, 2017

View reviewed changes

JobJob force-pushed the action_queues_redesign branch from f349770 to 82b6761 Compare April 27, 2017 13:03

This was referenced May 5, 2017

Fix Slider Signal string Test JuliaGizmos/Interact.jl#162

Closed

Tag Reactive.jl v0.4.0 JuliaLang/METADATA.jl#8931

Merged

JobJob force-pushed the action_queues_redesign branch from e158ce3 to e249069 Compare May 7, 2017 09:32

shashi merged commit 8e9d086 into JuliaGizmos:master May 7, 2017

JobJob deleted the action_queues_redesign branch May 18, 2017 07:31


		#### Filter

		Filter works by setting the filter node's active field to false when the filter condition is false. Downstream nodes (nodes later in the current `action_queue`) check if at least one of their parents has been active, if none of them have been active then the node will not run it's action, thus propagating the filter correctly.


		#### Flatten

		Implementation is a bit fiddly. See comments in the code for how the graph is rewired.


		Each Signal also has a field `active` which stores whether or not the node was active (had its actions run) in the current push. A Signal will be set to active if it is `push!`ed to, or if any of its parent `Signal`s were active.

		On processing each `push!`, we run through `nodes` and execute the actions of each node if any of its parents were active.

Node Creation Order Redesign #129

Node Creation Order Redesign #129

Conversation

JobJob commented Mar 24, 2017 • edited Loading

shashi commented Mar 24, 2017

SimonDanisch commented Mar 24, 2017

JobJob commented Mar 24, 2017

JobJob commented Mar 24, 2017

SimonDanisch commented Mar 24, 2017

JobJob commented Mar 24, 2017 • edited Loading

JobJob commented Mar 25, 2017

JobJob commented Mar 25, 2017

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JobJob commented Apr 19, 2017

JobJob commented Apr 21, 2017

JobJob commented Apr 23, 2017

JobJob commented Apr 24, 2017 • edited Loading

JobJob commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JobJob commented Apr 25, 2017

JobJob commented Apr 27, 2017 • edited Loading

shashi commented May 2, 2017

JobJob commented May 2, 2017 • edited Loading

JobJob commented May 7, 2017

shashi commented May 7, 2017

JobJob commented May 7, 2017

timholy commented May 7, 2017

timholy commented May 8, 2017

JobJob commented May 9, 2017

JobJob commented Mar 24, 2017 •

edited

Loading

JobJob commented Mar 24, 2017 •

edited

Loading

JobJob commented Apr 24, 2017 •

edited

Loading

JobJob commented Apr 27, 2017 •

edited

Loading

JobJob commented May 2, 2017 •

edited

Loading