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

Concurrent queue fixes #1835

Merged
merged 4 commits into from Jan 28, 2019

Conversation

4 participants
@shaan1337
Copy link
Member

shaan1337 commented Jan 25, 2019

Symptoms:

  • Random write speeds slow down (by ~6x). On my laptop (Core i7-7700HQ) write speeds drop down from 20k/s to 3k/s since concurrentQueue.Count is called in every iteration of QueuedHandlerMRES.

Thanks to @Lougarou and @avish0694 for helping out with tests and implementation of fix.

Affected Versions/OS:

  • Only 5.0.0-RC1 and 5.0.0-RC2
  • mono: Linux and macOS. Windows is not affected.

Issue:
This is a workaround for: dotnet/corefx#29759

As from mono 5.2 and .NET Core 2.0, ConcurrentQueue.Count has knowingly been made slower (in some cases, O(N)) in favor of faster Enqueue() and Dequeue() operations (See: dotnet/corefx#29759 (comment)). It can be very very slow in some cases as demonstrated by this proof of concept:
Program.cs.zip

How EventStore is affected
The queue count is required in most of our queues for queue length stats. It's also required in some places where the queue size is bounded by a max size (most notably in the ClientAPI)

Fixes

  • 6dea568 This PR implements a workaround by implementing a wrapper: ConcurrentQueueWrapper that maintains the queue count at the expense of a slightly slower Enqueue()/TryDequeue(). Interlocked.Increment/Interlocked.Decrement operations are of the order of 36-90 cycles (similar to a division operation) which is good enough in our case.
  • f860b50, cafd363 The wrapper is applied everywhere we're calling .Count on a ConcurrentQueue
  • fbd446d Every q.Count() > 0 check is replaced by !q.IsEmpty which is not affected by this issue.

@shaan1337 shaan1337 requested review from jageall and jen20 Jan 25, 2019

@jen20

jen20 approved these changes Jan 28, 2019

@avish0694 avish0694 self-requested a review Jan 28, 2019

@shaan1337 shaan1337 merged commit fedbd44 into master Jan 28, 2019

@shaan1337 shaan1337 deleted the concurrent-queue-fixes branch Jan 28, 2019

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