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

System.Threading.Channels : Expose Channel.EstimatedCount #30852

Closed
mgravell opened this issue Jul 5, 2018 · 24 comments · Fixed by dotnet/runtime#312
Closed

System.Threading.Channels : Expose Channel.EstimatedCount #30852

mgravell opened this issue Jul 5, 2018 · 24 comments · Fixed by dotnet/runtime#312

Comments

@mgravell
Copy link
Contributor

@mgravell mgravell commented Jul 5, 2018

This is already available internally via things like ItemsCountForDebugger, but it would be invaluable for system monitoring purposes.

Yes, I know about the inherent race condition of a Count; however, the purpose is not for making decisions; it isn't to decide between 0 or 2, nor is it to decide between 10000 or 10002 - it is so that when shit hits the fan, in my automated "what the hell happened" scanning, I can tell the difference between a backlog of 2 (probably fine) and 10002 (probably part of the problem).

Happy for it to be hidden via attribs, hard to find, given a name from hell (int RoughCountEstimateDontTrustThisSeriously()) - but I really really want it.

Happy to PR etc.


Edit: API proposal: add

public abstract int EstimatedCount {get;}

with suitable implementations, for example ala ItemsCountForDebugger.

It probably makes sense to change the debugger display attribute to use "EstimatedCount" and remove the extra non-public property.

@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Jul 5, 2018

I'm fine with such a property as long as we can come up with a name that is as much of a warning as Marc's "RoughCountEstimateDontTrustThisSeriously" but obviously not quite as verbose :)

@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Jul 5, 2018

cc: @tarekgh

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Jul 5, 2018

@mgravell can't you get this using the reflection?

@mgravell

This comment has been minimized.

Copy link
Contributor Author

@mgravell mgravell commented Jul 5, 2018

yes, but that's kinda defeating the purpose of an API, no?

edit: this type is essentially a queue; knowing the length of the queue is, to me as a consumer, a fundamental property of the type.

@Drawaes

This comment has been minimized.

Copy link
Collaborator

@Drawaes Drawaes commented Jul 5, 2018

Having (even a quick and dirty) backlog counts for queues is a hard requirement for any real system. We run a lightweight custom actor style model that we currently use Data flow for and are looking to move. One key feature for measuring system health is a background thread that goes around watching no actor is getting backed up and if so alerts operators....

@mgravell

This comment has been minimized.

Copy link
Contributor Author

@mgravell mgravell commented Jul 5, 2018

@stephentoub just for comparison... ConcurrentQueue<T> has all the same thread-race concerns; the name of the count property is... Count :)

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Jul 5, 2018

this type is essentially a queue; knowing the length of the queue is, to me as a consumer, a fundamental property of the type.

Using queue is an internal implementation of specific types of channels we support so if anyone else implements a custom channel, it is not necessary to be a queue. I am just thinking if providing this property will not be confusing in the future when providing more types of channels or even changing the internal implementation of current channels if we need to.

@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Jul 5, 2018

@mgravell, ConcurrentQueue provides snapshot semantics, such that the Count it provides is guaranteed to have been the number of items in the queue at some time (even if it's changed since). It actually makes its Count more expensive to be able to do that, and has implications on the rest of the collection's implementation. I would like to avoid such expectations here.

@NickCraver

This comment has been minimized.

Copy link
Member

@NickCraver NickCraver commented Jul 5, 2018

How about .ApproximateCount?

@mgravell

This comment has been minimized.

Copy link
Contributor Author

@mgravell mgravell commented Jul 5, 2018

I'd also be fine with bool TryGetApproximateCount(out int count) (or bool TryGetCount(out int count)) - and if the custom implementation can't provide that: then: don't! or an int?, whatever

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Jul 7, 2018

@stephentoub do you think having count property will make sense for any user's custom channels or for any channel we support later. if you think so, here is the suggested list for the name we may choose from:

  • Count
  • ApproximateCount
  • EstimatedCount
  • VolatileCount
  • UnsteadyCount
  • UnsafeCount
@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Jul 8, 2018

@stephentoub do you think having count property will make sense for any user's custom channels or for any channel we support later

I'm having trouble coming up with a case where the concept wouldn't make sense. The closest I've come is a channel that's not actually backed by anything, e.g. an InfiniteChannel, where a read will always succeed, in which case the count would effectively be infinite. But even in such a case you could argue that a Count of 1 always makes sense, such that there's always 1 item available, and the moment that one is consumed, another comes into existence. Otherwise, since a channel is all about being a producer/consumer data structure that allows a producer to hand off items to a consumer, and since that number of items should be "countable", it should be possible to always come up with a count.

The question for me isn't whether the concept is possible, but whether it'll always be possible to implement efficiently and safely. One of the problems we've had with Count members on concurrent collections in the past is that they're often much slower than developers expect, and as such developers end up calling Count expecting it to be O(1) and fast, but get perf issues as a result of their expectations not being met. We can alleviate that by choosing a name like one of the ones you suggest. However, there's still the possible issue that needing to support it could have an impact on the rest of the implementation. It's one thing if adding Count is pay-for-play, such that you only pay the costs of getting the count if you try to get the count; my larger concern would be if exposing Count was not pay-for-play and if doing so actually caused other things to get slower, e.g. if in order to support getting the count, enqueue and dequeue required more synchronization.

For example, when Channel.CreateUnbounded is used with ChannelOptions.SingleReader, the implementation will use a SingleProducerSingleConsumerQueue as the implementation: that queue not only has no locks, it also has no interlockeds, using only volatile operations to implement the required thread-safety for a single producer and a single consumer. The implementation is currently commented that it's only valid when nothing is concurrently mutating the collection:

/// <remarks>This method is not safe to use concurrently with any other members that may mutate the collection.</remarks>
internal int Count

We would need to review the code to understand why that is. Right now, it likely doesn't matter, as the count is only used when everything is frozen in the debugger. But if we expose this such that it may end up being used concurrently with either readers or writers, both of which mutate the collection, would the worst case just be an inaccurate count? If so, that's probably ok, as long as it's named correctly. Or would the worst case be some kind of infinite loop? That's not ok (if it can't be fixed). We would need to validate the worst case here.

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Jul 8, 2018

ok, let's choose a name to indicate that this count is not safe at all and would be the responsibility of the caller how to use it. I mentioned the name suggestions so far we can choose one of them or come up with one better name.

@Drawaes

This comment has been minimized.

Copy link
Collaborator

@Drawaes Drawaes commented Jul 8, 2018

I don't mind most of the names except for what my opinion is worth I would leave "unsafe" out of it. In the lexicon of .net it has a pretty specific meaning around memory. The count method will give a stronger guarantee than "I will leak memory in some circumstances" its more "I might be inaccurate".

@Joe4evr

This comment has been minimized.

Copy link

@Joe4evr Joe4evr commented Jul 8, 2018

EstimatedCount sounds like the best option, then.

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Jul 8, 2018

If all agree, @mgravell may need to get the API proposal written in the top of this page and we can proceed from here.

@mgravell mgravell changed the title System.Threading.Channels : Expose Channel.Count System.Threading.Channels : Expose Channel.EstimatedCount Jul 8, 2018
@mgravell

This comment has been minimized.

Copy link
Contributor Author

@mgravell mgravell commented Jul 8, 2018

@tarekgh like that? or did you mean something else?

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Jul 8, 2018

@mgravell here is some example regarding the format we prefer

#271

@tarekgh tarekgh modified the milestones: 3.0, Future Feb 16, 2019
@stephentoub stephentoub modified the milestones: Future, 5.0 Jul 18, 2019
@jafaber

This comment has been minimized.

Copy link

@jafaber jafaber commented Oct 5, 2019

A bit late, but if "Count" gives the wrong idea:

  • EstimatedCapacityUsed
  • EstimatedCapacityUsage

All I'm personally interested in is knowing when I hit say 90% capacity, so I can signal the producers to slow down.

@tarekgh

This comment has been minimized.

Copy link
Member

@tarekgh tarekgh commented Oct 6, 2019

@stephentoub I marked this as ready for review. please let me know if you you have different suggestions.

@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Oct 17, 2019

I've thought about this a bit more, and I think I'd actually be ok with just "Count". I still have all the same concerns, but on reflection I'm not sure that the more complicated naming options are actually safer in any meaningful way. So, whichever is fine.

@terrajobst

This comment has been minimized.

Copy link
Member

@terrajobst terrajobst commented Nov 26, 2019

Video

  • We considered naming the property with EstimatedCount but that seems overkill because you never get an exact count unless you freeze the world, but that's the nature of concurrent APIs.
  • We can't make the API abstract, but most of us believe that the API guideline is to use the Tester-Doer pattern (capability API + API).
namespace System.Threading.Channels
{
    public abstract partial class ChannelReader<T>
    {
        public virtual bool CanCount => false;
        public virtual int Count { get; }
    }
    public abstract partial class ChannelWriter<T>
    {
        public virtual bool CanCount => false;
        public virtual int Count { get; }
    }
}
@scalablecory

This comment has been minimized.

Copy link
Member

@scalablecory scalablecory commented Nov 26, 2019

@terrajobst what was decision re: throwing VS magic number?

@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Nov 26, 2019

Because we're adding the CanCount property, we then throw from Count if CanCount is false.

FWIW, as I just sat down to implement this quickly, I think the {Can}Count on ChannelWriter<T> is strange, as what you'd really want to know on the writer is how much room is available, not how many items you could consume, which you can't on a "writer". I'm going to skip the writer side for now and just do the reader. If we find a strong need later, we can revisit the writer side separately, but I think it would need a different API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.