Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add ConcurrentQueue<T> Clear method#13976

Closed
AlexRadch wants to merge 14 commits intodotnet:masterfrom
AlexRadch:ConcurrentQueue.Clear
Closed

Add ConcurrentQueue<T> Clear method#13976
AlexRadch wants to merge 14 commits intodotnet:masterfrom
AlexRadch:ConcurrentQueue.Clear

Conversation

@AlexRadch
Copy link
Copy Markdown

Implement ConcurrentQueue.Clear API. See https://github.com/dotnet/corefx/issues/2338

@karelz
Copy link
Copy Markdown
Member

karelz commented Nov 25, 2016

while (!IsEmpty)
{
Segment head = _head;
head.Clear();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a lot of additional code. Is it measurably more efficient than just doing:

T item;
while (TryDequeue(out item));

? It looks like it's essentially a copy of the TryDequeue/TryRemove implementation, just with Clear/Clear naming, so I wouldn't expect it would be, unless I'm missing something. We shouldn't be adding all of this extra complicated code unless it actually makes a meaningful difference.

Copy link
Copy Markdown
Author

@AlexRadch AlexRadch Nov 25, 2016

Choose a reason for hiding this comment

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

TryDequeue remove only one element per call see if (Interlocked.CompareExchange(ref _low, lowLocal + 1, lowLocal) == lowLocal) code.

But head.Clear() remove all elements in Segment per call see if (Interlocked.CompareExchange(ref _low, highLocal + 1, lowLocal) == lowLocal) code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. Ok. Please add a comment highlighting that fairly subtle difference between the two implementations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added comment and created tests. But I have issues with config files to add API correctly.

@AlexRadch
Copy link
Copy Markdown
Author

AlexRadch commented Nov 28, 2016

I added comment about Clear method.
I created tests for Clear method;
I do not clear understand Adding APIs Guidelines so test project can be compiled and tested locally in src\System.Collections.Concurrent\tests\ folder but global build return strange errors.
Can anybody help me with config files to add API correctly?

@Priya91
Copy link
Copy Markdown
Contributor

Priya91 commented Nov 28, 2016

@AlexRadch Can you cleanup the merge commits and have only your commits in history.. Also there's a compiler error: missing semicolon,

ConcurrentQueueTests.netstandard1.7.cs(58,43): error CS1002: ; expected [/mnt/resource/j/workspace/dotnet_corefx/master/centos7.1_debug_prtest/src/System.Collections.Concurrent/tests/System.Collections.Concurrent.Tests.csproj]

Please run tests locally, running build.cmd at root of repo, or for additional instructions here

@AlexRadch
Copy link
Copy Markdown
Author

I tried to remove excess commits but I did not find how to do this so I think I have only one way - recreate my corefx fork. So I will close all my PRs, will recreate my corefx fork and will create new PRs.

@stephentoub
Copy link
Copy Markdown
Member

As per #14084 (comment), it'll probably be easiest for you to wait for @justinvp to finish #13828 before proceeding with this change.

@AlexRadch
Copy link
Copy Markdown
Author

@stephentoub Ok. Thanks.

@karelz karelz modified the milestone: 1.2.0 Dec 6, 2016
@danmoseley
Copy link
Copy Markdown
Member

@AlexRadch I see #13828 is now merged so you should be unblocked to finish this PR.

@karelz
Copy link
Copy Markdown
Member

karelz commented Jan 4, 2017

@AlexRadch do you plan to finish it?

@karelz
Copy link
Copy Markdown
Member

karelz commented Jan 11, 2017

@AlexRadch any update?
Or would you prefer to close the PR for now and make the issue "up for grabs" again for others to pick?

@stephentoub
Copy link
Copy Markdown
Member

Thanks for your work on this, @AlexRadch. Since this has been sitting for a couple of months and needed to be significantly updated based on changes to the target types, I went ahead and took your commits and created a new PR at #15292.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants