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

What about adding a ConcurrentSet<T>? #6318

Open
JohanLarsson opened this issue Feb 22, 2016 · 25 comments

Comments

Projects
None yet
@JohanLarsson
Copy link

commented Feb 22, 2016

The need for it comes up every now and then. To me it would make sense next to ConcurrentDictionary<TKey, TValue>

@ayende

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

What does this gives you that ConcurrentDictionary<T, object> (and just not using the value) ?

@JohanLarsson

This comment has been minimized.

Copy link
Author

commented Feb 23, 2016

Nothing but communicates intent better imo. I usually do stuff like this (not a proper set).
Set operations are perhaps tricky to get right with concurrency.

@svick

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

@ayende Apart from the "it's clearer what this code does" reason and the precedent of having HashSet<T>, there is a small, but measurable, difference in performance and memory consumption.

A very simple test: when I add a million values to Dictionary<int, int>, it takes 42 ms and consumes 26.6 MB of memory, while with HashSet<int>, it takes 38 ms and consumes 21.3 MB.

I assume the difference with concurrent collections would be similar.

@JonHanna

This comment has been minimized.

Copy link
Collaborator

commented Feb 23, 2016

If anything more so, since the concurrency consideration means there are a few spots that want to be particularly efficient, so not having to do anything with values would be an even greater gain there than with HashSet<T>.

@svick

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

@JohanLarsson

Set operations are perhaps tricky to get right with concurrency.

I think a ConcurrentSet would be useful even without set operations.

@JonHanna

This comment has been minimized.

Copy link
Collaborator

commented Feb 23, 2016

I know of one C# concurrent hash set and one in Java and neither offer concurrency on set operations, only on the add, remove, contains.

@hermitdave

This comment has been minimized.

Copy link

commented Feb 23, 2016

Is there's plan to add this? If there is and it's up for grabs, I can look into it

@svick

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2016

@hermitdave You might want to have a look at how the API review process works.

@hermitdave

This comment has been minimized.

Copy link

commented Feb 23, 2016

Thanks @svick will keep an eye on this one

@JohanLarsson

This comment has been minimized.

Copy link
Author

commented Feb 23, 2016

@svick

I think a ConcurrentSet would be useful even without set operations.

Agreed, perhaps it should not be called Set? IDK.

@Priya91

This comment has been minimized.

Copy link
Member

commented Sep 30, 2016

@hermitdave Do you have a proposal for this ?

@hermitdave

This comment has been minimized.

Copy link

commented Sep 30, 2016

@Priya91 oops forgot about this one.. I'll get started asap

@hermitdave

This comment has been minimized.

Copy link

commented Oct 3, 2016

Starting work on this

@karelz

This comment has been minimized.

Copy link
Member

commented Nov 18, 2016

@hermitdave are you working on it?

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Dec 27, 2016

@hermitdave do you still have interest in writing up the formal proposal here? If we review okays it promply and someone is interested in implementation, there may still be time to get this into 2.0.

Possibly the implementation could begin by wrapping ConcurrentDictionary, so the API is quickly available to use, then rewritten to have the space/time improvements of a custom implementation.

@daveaglick

This comment has been minimized.

Copy link

commented Dec 27, 2016

FWIW, @i3arnon has a really nice implementation at https://github.com/i3arnon/ConcurrentHashSet

@hermitdave

This comment has been minimized.

Copy link

commented Jan 4, 2017

@i3arnon maybe you should do a PR instead.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

Just to be clear folks -- feel free to make a PR, but we can't take any change without an API review approval. That needs a proposal written up as above. @hermitdave were you going to do that?

@hermitdave

This comment has been minimized.

Copy link

commented Jan 4, 2017

@danmosemsft I had a look at what @i3arnon did and it is which is decent. if he isn't interested then I will do a PR later this week

@karelz

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

@hermitdave we first need API proposal (see API review process) - i.e. review the API surface of the collection (with motivation and relation to 'classic' HashSet and other concurrent collections).

Just to set expectations: Based on recent discussions around other collections, we will likely have to find the right place for new collections (CoreFX repo might not be the desired destination). That may take even longer.

@danmosemsft

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

@hermitdave apologies, I wasn't clear in my comment. I should have said -- feel free to prototype in a fork, but a PR against CoreFX would be noise at this point without API approval

@vancem

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

Just FYI The implementation Dan refers to above is very specialized (e.g. there is no remove), and is probably not representative.

The C# compiler community wrote a simple wrapper for themselves, that is probably a better representation of what would be done if we wanted to add this class.

http://source.roslyn.io/#microsoft.codeanalysis/InternalUtilities/ConcurrentSet.cs

@eltiare

This comment has been minimized.

Copy link

commented Jul 24, 2018

I find myself wanting this very much on occasion. Seems like something that shouldn't be overlooked.

@LeaFrock

This comment has been minimized.

Copy link

commented Feb 13, 2019

@vancem I can't agree more! But three years passed, and no ConcurrentHashSet yet =.=
I think most of us want a kind of collections which is similar to ConcurrentBag, but its elements are never duplicated and its 'Contains()' is an O(1) operation like HashSet.
We can make it using ConcurrentDictionary, but that implementation is too ugly.

@stephentoub stephentoub modified the milestones: Future, 5.0 Jul 18, 2019

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