Added BlockingQueue implementation based on JCtools #1682

merlimat · 2018-09-17T21:56:58Z

Motivation

Add a BlockingQueue implementation that is suitable for low latency and low contention.

Key points:

Optimized for multiple producers and single consumer
When waiting for entries, the thread is blocked with busy wait to avoid context switch.

(This will be used in subsequent PRs to optionally enable it)

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java

jvrao · 2018-09-17T22:43:46Z

@merlimat any perf/measurements that you can share?

dlg99 · 2018-09-18T00:38:09Z

I am curious why you decided to implement blocking queue on top of JCtools' MPSC vs using disruptor that implements it.
https://github.com/conversant/disruptor
https://github.com/conversant/disruptor/blob/master/src/test/java/com/conversantmedia/util/concurrent/DisruptorBlockingQueueTest.java

I do not have experience or stake in using either of those, but conversant claims to be faster and it already has blocking queue API.

eolivelli · 2018-09-18T05:15:19Z

Same curiosity from me. Even Netty has its own MCSP queue

eolivelli

Looks good to me +1

sijie · 2018-09-21T01:02:29Z

@merlimat can you answer the questions why we are choosing JCtools queues?

ivankelly · 2018-09-21T09:47:35Z

@sijie @merlimat I suspect JCtools was chosen because this code was written before everyone jumped on the lock-free bandwagon ;)

merlimat · 2018-11-08T08:07:43Z

@merlimat any perf/measurements that you can share?

@jvrao
Not isolated to this implementation. It's tricky to properly microbenchmark a concurrency queue.

This really brings 2 benefits:

Avoiding contention on the enqueuing threads (eg: IO threads passing items to journal, or submitting tasks the OrderedSafeExecutor)
Combined with a busy-poll strategy, it will avoid the latency penalty of context switch almost completely since the receiving thread will not "sleep" or wait for a new item in the queue, but it will be actively polling and never descheduled (this when also combining with CpuAffinity, see Added module to enable CPU affinity #1641).

Some test results were shared here: netty/netty#8267 (comment) . Though this was including Pulsar in the mix, on top of BK, with same kind of optimizations applied.

I am curious why you decided to implement blocking queue on top of JCtools' MPSC vs using disruptor that implements it.

@dlg99 @eolivelli I was not aware of conversant library. I was looking at Lmax disruptor but it forces to change all your code to fit into its model of dedicating a thread to it. I wanted to have a regular blocking queue so that this can also be directly plugged into the OrderedSafeExecutor (or any JDK executor), so that existing code doesn't have to be modified.

JCTool seems to be well maintained, the concurrency code is very scrupulously reviewed and is being used in several high profile projects, from Netty, Cassandra, Log4j, etc.

Same curiosity from me. Even Netty has its own MCSP queue

Netty is actually importing JCTools queue :)

merlimat · 2018-11-08T20:27:26Z

run integration tests
run bookkeeper-server replication tests

eolivelli · 2018-11-08T22:01:54Z

Yes I noticed that Netty has its own copy of jctools queue.

I am fine with this idea.
Thank you for working on this topic

eolivelli

I left two comments, please take a look.

Overall it looks good to me

eolivelli · 2018-11-08T22:03:34Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/collections/BusyWait.java

+            handle = MethodHandles.lookup().findStatic(Thread.class, "onSpinWait", MethodType.methodType(void.class));
+        } catch (Throwable t) {
+            // Ignore
+        }


We should log this error, so that it is clear that we are not able to leverage this feature

This is not strictly critical for performance, it's just a way to consume less CPU power.

Since it's not available in Java8, I'd rather keep this at debug level to avoid printing the warning every time (eg: even if busywait is not used).

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptorImpl.java

merlimat · 2018-11-09T19:04:04Z

run pr validation

merlimat · 2018-11-09T19:42:14Z

run pr validation

merlimat · 2018-11-09T22:22:50Z

@eolivelli PTAL again

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java

eolivelli

Very good

+1

merlimat · 2018-11-13T18:41:32Z

@jvrao @dlg99 @sijie @ivankelly Are we good to merge this one?

merlimat · 2018-11-13T19:39:37Z

run pr validation
run integrations tests

jvrao · 2018-11-13T19:56:19Z

@dlg99 can you please review this? I wish we have some way to validate these perf changes. At lest to make sure it is not introducing perf regression. These changes are tricky - on paper they look like obvious win but in practice it is totally different picture. My comments are not specific to this change, but general category. @sijie

merlimat · 2018-11-13T19:57:50Z

@jvrao all these changes are by default turned off

sijie

lgtm if it is not turned on by default.

merlimat · 2018-11-14T00:40:53Z

run integration tests

merlimat · 2018-11-14T06:31:32Z

run integration tests

merlimat · 2018-11-14T23:10:07Z

IGNORE IT CI

merlimat · 2018-11-14T23:11:08Z

// Flaky TestCompatUpgrade integration test. Seen failing in other PRs as well

merlimat added the type/improvement label Sep 17, 2018

merlimat added this to the 4.9.0 milestone Sep 17, 2018

merlimat self-assigned this Sep 17, 2018

merlimat requested review from jvrao, ivankelly, sijie, jiazhai and eolivelli September 17, 2018 21:56

jvrao reviewed Sep 17, 2018

View reviewed changes

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessorV3.java Outdated Show resolved Hide resolved

eolivelli approved these changes Sep 18, 2018

View reviewed changes

Added BlockingQueue implementation based on JCtools

ade756a

merlimat force-pushed the blocking-queue branch from f542515 to ade756a Compare November 8, 2018 07:37

merlimat added 2 commits November 8, 2018 00:10

Fixed checkstyle

0599e7a

Removed unused import

6d12764

eolivelli requested changes Nov 8, 2018

View reviewed changes

Addressed comments

b7ab005

eolivelli reviewed Nov 9, 2018

View reviewed changes

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java Outdated Show resolved Hide resolved

Removed interruped exception from signature

72d325e

merlimat force-pushed the blocking-queue branch from cfef277 to 72d325e Compare November 10, 2018 01:09

eolivelli approved these changes Nov 10, 2018

View reviewed changes

sijie approved these changes Nov 13, 2018

View reviewed changes

Merge remote-tracking branch 'apache/master' into blocking-queue

e4306e1

merlimat force-pushed the blocking-queue branch from 8bff8ec to e4306e1 Compare November 13, 2018 22:13

Added jctools to LICENSE-bkctl.bin.txt as well

35ceb66

merlimat added area/bookie area/client release/4.9.0 labels Nov 14, 2018

merlimat merged commit 5997963 into apache:master Nov 14, 2018

merlimat deleted the blocking-queue branch November 14, 2018 23:32

dlg99 mentioned this pull request Oct 12, 2021

Journal thread fetch entry from queue cost too much time on acquireInterruptibly #2820

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added BlockingQueue implementation based on JCtools #1682

Added BlockingQueue implementation based on JCtools #1682

merlimat commented Sep 17, 2018

jvrao commented Sep 17, 2018

dlg99 commented Sep 18, 2018

eolivelli commented Sep 18, 2018

eolivelli left a comment

sijie commented Sep 21, 2018

ivankelly commented Sep 21, 2018

merlimat commented Nov 8, 2018

merlimat commented Nov 8, 2018

eolivelli commented Nov 8, 2018

eolivelli left a comment

eolivelli Nov 8, 2018

merlimat Nov 9, 2018

merlimat commented Nov 9, 2018

merlimat commented Nov 9, 2018

merlimat commented Nov 9, 2018

eolivelli left a comment

merlimat commented Nov 13, 2018

merlimat commented Nov 13, 2018

jvrao commented Nov 13, 2018

merlimat commented Nov 13, 2018

sijie left a comment

merlimat commented Nov 14, 2018

merlimat commented Nov 14, 2018

merlimat commented Nov 14, 2018 •

edited

Loading

merlimat commented Nov 14, 2018

Added BlockingQueue implementation based on JCtools #1682

Added BlockingQueue implementation based on JCtools #1682

Conversation

merlimat commented Sep 17, 2018

Motivation

jvrao commented Sep 17, 2018

dlg99 commented Sep 18, 2018

eolivelli commented Sep 18, 2018

eolivelli left a comment

Choose a reason for hiding this comment

sijie commented Sep 21, 2018

ivankelly commented Sep 21, 2018

merlimat commented Nov 8, 2018

merlimat commented Nov 8, 2018

eolivelli commented Nov 8, 2018

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Nov 8, 2018

Choose a reason for hiding this comment

merlimat Nov 9, 2018

Choose a reason for hiding this comment

merlimat commented Nov 9, 2018

merlimat commented Nov 9, 2018

merlimat commented Nov 9, 2018

eolivelli left a comment

Choose a reason for hiding this comment

merlimat commented Nov 13, 2018

merlimat commented Nov 13, 2018

jvrao commented Nov 13, 2018

merlimat commented Nov 13, 2018

sijie left a comment

Choose a reason for hiding this comment

merlimat commented Nov 14, 2018

merlimat commented Nov 14, 2018

merlimat commented Nov 14, 2018 • edited Loading

merlimat commented Nov 14, 2018

merlimat commented Nov 14, 2018 •

edited

Loading