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

Use BlockingCollection in SharedTextWriter #1427

Merged
merged 2 commits into from
Apr 9, 2019

Conversation

bjorkstromm
Copy link
Member

Did some profiling and noticed a lot of time was spent on ShareTextWriter.WriteLine and SharedTextWriter.WriteLineAsync.

Noticed the lock and figured we could use SemaphoreSlim instead.

@bjorkstromm
Copy link
Member Author

image

@bjorkstromm
Copy link
Member Author

Could do some benchmarks to see if there's any actual performance boost.

@david-driscoll
Copy link
Member

I just figure if we can avoid blocking a thread at all it would probably be better.

@rchande
Copy link

rchande commented Mar 21, 2019

@mholo65 What do you think of implementing this with BlockingCollection<T>? https://docs.microsoft.com/en-us/dotnet/api/system.collections.concurrent.blockingcollection-1?view=netcore-3.0

@david-driscoll
Copy link
Member

@rchande
Copy link

rchande commented Mar 22, 2019

Actually, blockingCollection blocks a thread, so this is probably better.

@david-driscoll
Copy link
Member

A blocking collection will only block it's own thread, not the calling task, assuming you just queue the item up. For input / output it probably makes more sense to just have the dedicated thread, then we can queue data up for it as fast as we're allowed, but it'll write out at whatever rate it can.

As long as the order is guaranteed (which by default BlockingCollection uses ConcurrentQueue) I think dumping handling the writing in another thread might make sense.

Thoughts? @mholo65 @rchande

As it is I'm fine with this change, but it might almost make more sense to refactor it entirely.

@bjorkstromm
Copy link
Member Author

@david-driscoll @rchande I think that BlockingCollection is even better, then we'll dump the IO to another thread, and won't block main thread.

@bjorkstromm bjorkstromm changed the title Use SemaphoreSlim instead of lock object in SharedTextWriter Use BlockingCollection in SharedTextWriter Apr 4, 2019
@bjorkstromm
Copy link
Member Author

@david-driscoll @rchande @filipw I modified this PR to use BlockingCollection instead, also no need for the Async version anymore.

@filipw filipw merged commit eab3db1 into OmniSharp:master Apr 9, 2019
@filipw
Copy link
Member

filipw commented Apr 9, 2019

thanks 👏

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.

4 participants