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.

Sign up for GitHub

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

Jump to bottom

Channel constructor requires an explicit size. Also implements 0-sized channels #18832

Merged
merged 1 commit into from
Oct 21, 2016
Merged

Channel constructor requires an explicit size. Also implements 0-sized channels #18832

merged 1 commit into from
Oct 21, 2016

Conversation

amitmurthy
Copy link
Contributor

As discussed in #17698 (comment), the default channel size of 32 has been removed. The size needs to be specified explicitly. Channel(Inf) is an unlimited channel, a shorter way of writing Channel(typemax(UInt))

Channel tests have been moved into its own file.

Added support for 0-sized channels (as mentioned in #17699). This provides the same behavior as produce/ consume which can be deprecated in a separate PR.

@amitmurthy amitmurthy mentioned this pull request Oct 7, 2016
Closed
@kshyatt kshyatt added parallelism Parallel or distributed computation io Involving the I/O subsystem: libuv, read, write, etc. labels Oct 7, 2016
tkelman
tkelman reviewed Oct 8, 2016
View reviewed changes
base/channels.jl
put!(c,v,Val{c.sz_max==0})
end

function put!(c::Channel, v, ::Type{Val{false}})
Copy link
Contributor

Choose a reason for hiding this comment

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

add some comments about what this new Val argument is differentiating?

tkelman
tkelman reviewed Oct 8, 2016
View reviewed changes
base/docs/helpdb/Base.jl
@@ -1437,10 +1437,16 @@ endof
Constructs a `Channel` that can hold a maximum of `sz` objects of type `T`. `put!` calls on
a full channel block till an object is removed with `take!`.

`Channel(0)` constructs a Channel without a backing store. Consequently a `put!` on a
0-sized channel will block till another task calls a `take!` on it. And vice-versa.
Copy link
Contributor

Choose a reason for hiding this comment

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

"block until"

tkelman
tkelman reviewed Oct 8, 2016
View reviewed changes
base/docs/helpdb/Base.jl
`Channel(0)` constructs a Channel without a backing store. Consequently a `put!` on a
0-sized channel will block till another task calls a `take!` on it. And vice-versa.

`isready` on a 0-sized channel returns true if there are any tasks blocked on a `put!`
Copy link
Contributor

Choose a reason for hiding this comment

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

should be separated with either a period or another line between this and the following statement

@amitmurthy
Copy link
Contributor Author

@tkelman thanks for the feedback. Have pushed a commit that makes the arguments self-explanatory and also updated the docs.

@tkelman
Copy link
Contributor

tkelman commented Oct 9, 2016

several more cases of "block till" would be better as "block until" I think

@amitmurthy
Copy link
Contributor Author

@StefanKarpinski your thoughts?

fetch on an unbuffered channel is not supported. put!, take and wait work as expected.

@StefanKarpinski
Copy link
Member

Yes, in general this is good. A couple of high-level things.

It feels like Int might be a better type for the buffer size. We don't really need the high bit for this – real buffer sizes are going to be much smaller than this and we generally use Int for all sizes of things and indices into them. The constructor can still check that the size isn't negative.

Is all the Val stuff for buffered versus unbuffered really necessary? It seems like it's for performance but since it's not captured in the type of the channel objects, dispatch seems like it will end up being dynamic anyway, in which case some if/elses are going to be faster anyway. And this is for operations that seem like they'll swamp the cost of some if/elses anyway.

@amitmurthy
Copy link
Contributor Author

In this case, the dispatch on values was mainly for style and readability. Will capture it in the type or replace with if/else.

@amitmurthy
Copy link
Contributor Author

Updated.

@amitmurthy
Copy link
Contributor Author

Deprecated Channel() instead of throwing an error.

Will squash and merge in a couple of days if there are no more comments.

tkelman
tkelman reviewed Oct 18, 2016
View reviewed changes
base/channels.jl

# deprecated empty constructor
function Channel()
depwarn(string("The empty constructor Channel() is deprecated. ",
Copy link
Contributor

@tkelman tkelman Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

why isn't this in deprecated.jl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Channel{T}() constructor needs to be inside the type definition, while Channel() which is equivalent of Channel{Any}() can be outside. Thus added both in channels.jl itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

why can't it be an outer constructor? especially with two different signatures in different places that should be removed, those comments are going to be easy to miss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How?

Channel() = Channel{Any}(32) is fine.
Channel{T}() = Channel{T}(32) is not callable.

julia> type Foo{T}
         foo::T
       end

julia> Foo{T}() = Foo{T}(1)
WARNING: static parameter T does not occur in signature for Type at REPL[2]:1.
The method will not be callable.
Foo{T}

@amitmurthy
Channel constructor requires an explict size.
e23f4e2 
Move channel tests into its own file.
Implement 0-sized channels.
@amitmurthy
Copy link
Contributor Author

@tkelman AV error is unrelated and OSX build is not starting up at all. Should I restart the build?

@amitmurthy amitmurthy merged commit f3b40bf into master Oct 21, 2016
@yuyichao yuyichao deleted the amitm/channel0 branch October 21, 2016 03:46
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 18, 2017
@Sacha0
Add NEWS.md entry for zero-arg Channel constructor dep (JuliaLang#18832…
cba0c69 
…).
@Sacha0 Sacha0 added deprecation This change introduces or involves a deprecation needs news A NEWS entry is required for this change labels May 18, 2017
ararslan pushed a commit that referenced this pull request May 19, 2017
@Sacha0 @ararslan
Add NEWS.md entry for zero-arg Channel constructor dep (#18832). (#21932
a72aad4 
)
@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label May 19, 2017
tkelman pushed a commit that referenced this pull request Jun 3, 2017
@Sacha0 @tkelman
Add NEWS.md entry for zero-arg Channel constructor dep (#18832). (#21932
effa3bb 
)

(cherry picked from commit a72aad4)
@fredrikekre fredrikekre mentioned this pull request Oct 10, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

Assignees
No one assigned
Labels
deprecation This change introduces or involves a deprecation io Involving the I/O subsystem: libuv, read, write, etc. parallelism Parallel or distributed computation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amitmurthy @tkelman @StefanKarpinski @kshyatt @Sacha0