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

add navailable()/capacity(RemoteRef{Channel}) #12781

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Aug 24, 2015

Make current number of elements in the Channel and its capacity available via RemoteRef.
Should be important for dynamically balancing the channel's usage in some applications.

@vtjnash
Copy link
Member

vtjnash commented Aug 25, 2015

anyone with direct experience with RemoteRef / RemoteValue want to chime in here? otherwise, this sounds reasonable to me.

@@ -102,6 +102,8 @@ end

eltype{T}(::Type{Channel{T}}) = T

capacity(c::Channel) = c.sz_max
Copy link
Member

Choose a reason for hiding this comment

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

should this be length(Channel) perhaps? capacity is not currently defined anywhere in Base.

will also need to add docs and tests for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also n_avail() (so far used in the context of Channel only). To me length() is rather a synonym of n_avail(). capacity() could be potentially reused in other places to return what sizehint!() has set. But I would be glad to change it into anything core Julia developers decide.

@amitmurthy
Copy link
Contributor

It used to be defined as length and size for "available" and "capacity" respectively. Jeff had removed size and renamed length in 2207c2f .

The change looks OK, but the names will go through a round of bikeshedding I guess.

@alyst alyst force-pushed the expose_channel_state branch from 7e846e3 to b79fa22 Compare August 26, 2015 20:42
@ViralBShah
Copy link
Member

@amitmurthy You should probably merge if you're ok with it.

@malmaud
Copy link
Contributor

malmaud commented Aug 27, 2015

If we're going to add capacity, it should probably be exported. If it's only intended for internal use, then I don't see the advantage over directly accessing the relevant fields.

I think the same is probably true for n_avail, although that's not germane for this PR.

@alyst
Copy link
Contributor Author

alyst commented Aug 27, 2015

IMHO, as capacity and current occupancy have impact on the user task scheduling (via put!()/take!()), it is essential to make them available to the user. Also, the capacity is specified upon channel creation, so it would be strange to conceal it later.

@malmaud
Copy link
Contributor

malmaud commented Aug 27, 2015

I'm just saying that capacity and n_avail should be exported in base/exports.jl.

@alyst
Copy link
Contributor Author

alyst commented Aug 27, 2015

I have left exporting out of the PR scope to increase the chances of merging. ;)

@malmaud
Copy link
Contributor

malmaud commented Aug 27, 2015

:) I guessed that, but that's exactly what I'm concerned about. The
decision either needs to be made that capacity et al should become part
of the Channel/RemoteRef API and so be exported, or that it shouldn't. The
documentation for channels can't tell users to reach into Base and access
unexported symbols because some developers weren't confident they were a
good idea.

On Thu, Aug 27, 2015 at 6:18 PM, Alexey Stukalov notifications@github.com
wrote:

I have left exporting out of the PR scope to increase the chances of
merging. ;)


Reply to this email directly or view it on GitHub
#12781 (comment).

@alyst
Copy link
Contributor Author

alyst commented Aug 27, 2015

Actually, for refining the API it would be nice to have some reference implementation of, say, master/slave model. It would also help the users a lot.

@amitmurthy
Copy link
Contributor

We can rename http://docs.julialang.org/en/latest/stdlib/io-network/#Base.nb_available to available and use the same name here?

size can mean "capacity" ?

@alyst
Copy link
Contributor Author

alyst commented Sep 1, 2015

Just some thoughts:
From the name it's not 100% obvious that available() returns a number, not a boolean.
For arrays size() returns a tuple, length() returns a number, so to maintain this "type stability" for channels, it's better to use length().
OTOH for arrays and other data structures both length() and size() report the number of available/accessible elements, it would be somewhat confusing if for channels this function returns something different.

@amitmurthy
Copy link
Contributor

navailable for both then? Given the reluctance to use underscores in exported functions...

Don't have an alternative for "capacity" given your observations....

@alyst
Copy link
Contributor Author

alyst commented Sep 1, 2015

+1 for navailable() and capacity()

@alyst
Copy link
Contributor Author

alyst commented Sep 1, 2015

IOBuffer calls "capacity" max_size in ctor, but doesn't define the getter function (yet?). So it could also be max_size() (keeping in mind size()/length(), however).

@amitmurthy
Copy link
Contributor

I think there is an unstated consensus on not using underscores in exported names. I don't know the rationale for the same, though personally I am OK with it.

@alyst alyst force-pushed the expose_channel_state branch 2 times, most recently from 51217c7 to 6c0a3b4 Compare September 11, 2015 11:43
@alyst
Copy link
Contributor Author

alyst commented Sep 11, 2015

It looked like navailable()/capacity() variant was the closest to consensus so far, so I've updated the PR with these names and added docs and exports.

Actually, since Channel concept bears some similarity to IOBuffer, nb_available(IOBuffer) could be renamed to navailable() and capacity(IOBuffer) could be used to report IOBuffer.maxsize.

It would be nice to have it in 0.4.

@alyst alyst changed the title add n_avail()/capacity(RemoteRef{Channel}) add navailable()/capacity(RemoteRef{Channel}) Sep 11, 2015
@alyst alyst force-pushed the expose_channel_state branch from 6c0a3b4 to 39ab92f Compare September 11, 2015 12:00
@StefanKarpinski
Copy link
Member

The navailable API has always struck me as a bad one that doesn't really match with the way we do I/O with coroutines. A coroutine should ask for a certain number of values and block until it gets that many values (maybe with upper and lower bounds). If the source of data is closed without getting that many values, the blocked call should raise an error which can be caught and handled appropriately. Asking about the capacity of something seems fine.

@alyst
Copy link
Contributor Author

alyst commented Sep 14, 2015

In my case workers are constantly sending elements to the channel. I use navailable() to get the number of elements at given moment. I can read them one by one, but it looked more effective to read it in chunks.

@StefanKarpinski
Copy link
Member

This certainly isn't the only place where the API occurs, so maybe it's fine, but I'd like to investigate removing it and replacing it with something better in the future.

@alyst
Copy link
Contributor Author

alyst commented Jan 21, 2018

[n]bytesavailable() discussion (#25634) reminded me of this limbo dweller.
n_avail(Channel) is still there, so maybe it's time to make a decision for 1.0.

@vtjnash
Copy link
Member

vtjnash commented Jun 26, 2019

Needs rebase and adding to docs/src, but still seems reasonable to me.

@vtjnash
Copy link
Member

vtjnash commented Jan 31, 2024

bump? This is one of the oldest open PRs now

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.

6 participants