Automatic channel creation on network Connect()
trustmaster opened this issue · 17 comments
What annoyed me originally with GoFlow is that you need to create a channel manually every time you connect 2 processes in a graph:
net.Connect("src", "Out", "dst", "In", make(chan string))It would be better if GoFlow inferred the channel type using reflection and created it on its own, so the above code could be simplified just to this:
net.Connect("src", "Out", "dst", "In")Back in 2012 this was not possible because Go's reflect didn't let you create a bidirectional channel out of a unidirectional. Nowadays it is possible, so we can give it another try.
I've implemented this on a separate branch connect_auto_chan_creation, please give it a try.
This method has a downside though: it is not possible to set buffer capacity per channel, you can only set the default one for all connections:
flow.DefaultBufferSize = 1(The default value set in the flow package itself is 32)
It would be nice to pass the buffer capacity as an optional argument to the Connect() function but Go doesn't support optional arguments. So it needs some kind of workaround.
Cool! Will test out ... but yeah, as you say, it would be quite desirable with per-channel buffer capacities, as it seems one can often tune the performance of the system quite a bit with those.
Works flawlessly, AFAIS!
As there are no default parameter values in Go, we should probably use 2 different functions to create a connection with a default buffer size or with custom one:
net.Connect("src", "srcPort", "dst", "dstPort")
net.ConnectBuf("src", "srcPort", "dst", "dstPort", 32)The question is: should the channels be buffered or non-buffered by default and what default buffer size is fine. If we use async processes by default, then probably non-buffered channels should be fine and lighter on memory footprint.
+1 for that. Was thinking about the same (a custom method), but didn't have any good naming suggestion. ConnectBuf sounds good, I think.
I also agree with non-buffered channels as default, as it probably gives the least surprise to newcomers ... If there's not a too big performance hit with using unbuffered channels, that is...
The main problem with unbuffered channels is that it makes synchronous networks need to run in lockstep, which probably decreases overall performance quite a bit.
But as you say, if async processes are the default, it is probably better with non-buffered channels anyway, complemented with clear documentation on the recommended combination of async/sync with buffered/unbuffered channels.
In just a few words, buffer tweaks vs. performance is tricky, it depends on throughput of each node in the network. In actor-like async mode it is even more tricky because goroutine stack acts as a kind of buffer of its own.
In just a few words, buffer tweaks vs. performance is tricky,
it depends on throughput of each node in the network
Yeah, I can imagine that is the case (and actually I had the same results, from playing around so far).
In actor-like async mode it is even more tricky because
goroutine stack acts as a kind of buffer of its own.
Aha, right!
This feature has now been merged into the master branch. This means that dependent code should be updated to either use Graph.Connect() with just 4 arguments or use Graph.ConnectBuf() instead.
It would be nice to pass the buffer capacity as an optional argument to the Connect() function but Go doesn't
support optional arguments. So it needs some kind of workaround.
I happened to need this and a search in the std lib http://golang.org/search?q=Options found some ways of doing it (kind of) see http://golang.org/pkg/net/http/cookiejar/#New or http://golang.org/pkg/image/jpeg/#Encode .
Maybe it's what you are looking for. if not sorry for the noise.
thanks for your work on goflow.
Thanks for pointing to that. Yes, passing a struct of options is the Go way around optional arguments. But in this specific case, is
net.Connect("src", "srcPort", "dst", "dstPort", nil)
net.Connect("src", "srcPort", "dst", "dstPort", &flow.ConnectOptions{BufferSize: 32})better than
net.Connect("src", "srcPort", "dst", "dstPort")
net.ConnectBuf("src", "srcPort", "dst", "dstPort", 32)?
In personal opinion, I tend to think that the latter option (net.ConnectBuf) will be more self-documenting and straight-forward (imagining how this will look in godoc), if there are not any really concrete benefits of the former?
Well, I should admit that I'm not really experienced in library design, so I should probably better shut up anyway, and leave the decision to others :)
The benefit of the 1st approach is using more than 1 optional parameter, e.g.
net.Connect("src", "srcPort", "dst", "dstPort", &flow.ConnectOptions{BufferSize: 32, Name: "Some connection"})I have a feeling that the library API will change several times along the way until we reach something "stable". For now I'd stick with ConnectBuf(), but if we add more parameters later, then it will be worth reconsidering the use of options struct.
Well, I should admit that I'm not really experienced in library design, so I should probably better shut up anyway :)
@samuell Well, for me I'll admit that I'm not at all experienced in coding, so I should shut up right now :) I'm just learning every day a bit more.
@trustmaster thanks again
Well, it is nice that we get this functionality, in any way! :) Keep up the good work, @trustmaster !