[Improve][Client] Group writing into the channel in PerChannelBookieClient #3886

hangc0276 · 2023-03-26T04:35:33Z

Motivation

When the BookKeeper client writes an entry to the BookKeeper server, it runs with the following steps:

Step 1: Initiate a PendingAddOp object.
Step 2: For each replica, select a bookie client channel according to the ledgerId
Step 3: Write the entry to the bookie client channel, and flush it.
Step 4: The entry was added to Netty's pending queue, and processed with the configured Netty pipeline, such as bookieProtoEncoder, lengthbasedframedecoder, and consolidation
Step 5: Waiting for the written response

If the bookie client writes small entries with high ops and the Netty's pending queue will be full and the Netty thread will be busy with processing entries and flushing them into the socket channel. The CPU will switch between the user mode and the kernel mode in high frequency.

#3383 introduced Netty channel flushes consolidation to mitigate syscall overhead. But it can not reduce the overhead on the Netty threads.

We can tune it one Step 3 to group the small entries into one ByteBuf and flush it into the Netty pending queue when conditions are met.

Design

When a new entry comes to the bookie client channel, we add it into one ByteBuf and check whether the ByteBuf exceeds the max threshold, the default is 1MB.

In order to avoid entry staying in the Bookie client channel ByteBuf for a long time causing high write latency, we schedule a timer task to flush the ByteBuf every 1 ms.

Performance

We test the write performance on my laptop with the following command.

bin/benchmark writes -ensemble 1 -quorum 1 -ackQuorum 1 -ledgers 100 -throttle 300000 -entrysize 60 -useV2 -warmupMessages 1000000

The performance result.

Writer ledgers	batched write ops/s	non-batched write ops/s	improved
1	333238	335970	0%
50	261605	153011	71%
100	260650	126331	100%
500	265628	164393	62%

zymap · 2023-03-27T10:54:49Z

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

@@ -349,6 +353,10 @@ enum ConnectionState {
    private final SecurityHandlerFactory shFactory;
    private volatile boolean isWritable = true;
    private long lastBookieUnavailableLogTimestamp = 0;
+    private ByteBuf pendingSendRequests = null;
+    private final Set<CompletionKey> pendingSendKeys = new HashSet<>();
+    private int maxPendingRequestsSize = DEFAULT_PENDING_REQUEST_SIZE;


Do we need to make it configurable?

zymap · 2023-03-27T10:55:09Z

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

-                } else {
-                    nettyOpLogger.registerFailedEvent(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS);
+                if (nextScheduledFlush == null) {
+                    nextScheduledFlush = channel.eventLoop().scheduleWithFixedDelay(this::flushPendingRequests,


Do we need to make the delay time configurable?

We should cancel the nextScheduledFlush in the close method.

Do we need to make the delay time configurable?

We should ensure the flush operation as soon as possible, or it may lead to the addEntry operation timeout. So if we make it configurable, the user may config it with a wrong value.

zymap · 2023-03-27T11:00:29Z

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

+
+    public synchronized void flushPendingRequests() {
+        final long startTime = MathUtils.nowInNano();
+        Set<CompletionKey> keys = new HashSet<>(pendingSendKeys);


Do we need to check the pendingSendRequests is not null before pending the listener? To avoid unnecessary listeners registered.

horizonzy · 2023-03-27T15:30:36Z

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

@@ -349,6 +353,10 @@ enum ConnectionState {
    private final SecurityHandlerFactory shFactory;
    private volatile boolean isWritable = true;
    private long lastBookieUnavailableLogTimestamp = 0;
+    private ByteBuf pendingSendRequests = null;
+    private final Set<CompletionKey> pendingSendKeys = new HashSet<>();


Here we can also use ObjectHashSet

horizonzy · 2023-03-27T16:26:05Z

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

+                    }
+                });
+                channel.writeAndFlush(request, promise);
+            }
        } catch (Throwable e) {
            LOG.warn("Operation {} failed", StringUtils.requestToString(request), e);
            errorOut(key);


The catch code block is not suitable for the new logic. It just handle the single key case, and didn't handle the pending keys case.
And in the try code block, it looks like there is no exception that will be thrown. I think we can remove the try catch block.

horizonzy · 2023-03-27T17:01:02Z

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

+        return pendingSendRequests.readableBytes() > MAX_PENDING_REQUEST_SIZE;
+    }
+
+    public synchronized void flushPendingRequests() {


The flushPendingRequests trigger at every millisecond. It may exacerbate the lock race between prepareSendRequests and flushPendingRequests.

Here we can refer to the implements of writeCache and writeCacheBeingFlushed in the SingleDirectoryDbLedgerStorage to make flushPendingRequests lock-free.

horizonzy · 2023-03-28T06:07:07Z

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

+        return pendingSendRequests.readableBytes() > MAX_PENDING_REQUEST_SIZE;
+    }
+
+    public synchronized void flushPendingRequests() {


Here we can refer to the implements of writeCache and writeCacheBeingFlushed in the SingleDirectoryDbLedgerStorage to make flushPendingRequests lock-free.

dlg99 · 2023-03-29T22:17:45Z

I think this should be configurable.
It improves throughput for some workloads, it is possible that more latency-sensitive workloads would want to disable this

dlg99 · 2023-03-29T22:24:14Z

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

+        public static void serializeAddRequests(Object request, ByteBuf buf) {
+            if (request instanceof ByteBuf) {
+                ByteBuf r = (ByteBuf) request;
+                buf.writeBytes(r);


what if buf does not have enough space to write bytes?

do we really need to copy data to another ByteBuf or can use CompositeByteBuf?

dlg99 · 2023-03-29T22:24:53Z

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

@@ -173,6 +174,9 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter {
                        BKException.Code.DuplicateEntryIdException,
                        BKException.Code.WriteOnReadOnlyBookieException));
    private static final int DEFAULT_HIGH_PRIORITY_VALUE = 100; // We may add finer grained priority later.
+    private static final int DEFAULT_PENDING_REQUEST_SIZE = 1024;
+
+    private static final int MAX_PENDING_REQUEST_SIZE = 1024 * 1024;


this cannot be larger than nettyMaxFrameSizeBytes

dlg99 · 2023-03-29T22:27:22Z

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

+        if (pendingSendRequests == null) {
+            pendingSendRequests = channel.alloc().directBuffer(maxPendingRequestsSize);
+        }
+        BookieProtoEncoding.RequestEnDeCoderPreV3.serializeAddRequests(request, pendingSendRequests);


what if request's size > maxPendingRequestsSize ?

dlg99 · 2023-03-29T22:51:02Z

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

-                } else {
-                    nettyOpLogger.registerFailedEvent(MathUtils.elapsedNanos(startTime), TimeUnit.NANOSECONDS);
+                if (nextScheduledFlush == null) {
+                    nextScheduledFlush = channel.eventLoop().scheduleWithFixedDelay(this::flushPendingRequests,


nextScheduledFlush is volatile so we assume access from different threads.
It can be null in "if" and not null in the assignment

eolivelli · 2024-05-03T08:03:06Z

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

+                buf.add((ByteBuf) request);
+            } else if (request instanceof ByteBufList) {
+                buf.add((ByteBufList) request);
+            }


else throw IllegalStateException ?

hangc0276 requested review from dlg99, eolivelli, merlimat and zymap March 26, 2023 04:35

hangc0276 self-assigned this Mar 26, 2023

hangc0276 added area/client type/improvement labels Mar 26, 2023

hangc0276 added this to the 4.17.0 milestone Mar 26, 2023

hangc0276 changed the title ~~[Improve][Client] Group writing for per bookie client~~ [Improve][Client] Group writing for perBookieClient Mar 26, 2023

hangc0276 changed the title ~~[Improve][Client] Group writing for perBookieClient~~ [Improve][Client] Group writing into the channel in PerChannelBookieClient Mar 26, 2023

zymap reviewed Mar 27, 2023

View reviewed changes

horizonzy reviewed Mar 27, 2023

View reviewed changes

horizonzy approved these changes Mar 28, 2023

View reviewed changes

dlg99 reviewed Mar 29, 2023

View reviewed changes

hangc0276 added 7 commits April 2, 2023 21:53

group write for perBookieClient

4cc7dbb

format code

c1da2a3

group write for perBookieClient

b496c15

format code

d5ad526

fix a bug

2417b4e

tmp

98b844d

use new impl

ab95ba6

hangc0276 force-pushed the chenhang/group_writing_for_perBookieClient branch from 00567fd to ab95ba6 Compare April 2, 2023 13:56

address comments

4ae8e53

eolivelli removed this from the 4.17.0 milestone May 3, 2024

eolivelli requested changes May 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improve][Client] Group writing into the channel in PerChannelBookieClient #3886

[Improve][Client] Group writing into the channel in PerChannelBookieClient #3886

hangc0276 commented Mar 26, 2023

zymap Mar 27, 2023

zymap Mar 27, 2023

horizonzy Mar 27, 2023

horizonzy Mar 28, 2023

zymap Mar 27, 2023

horizonzy Mar 27, 2023

horizonzy Mar 27, 2023

horizonzy Mar 27, 2023

horizonzy Mar 28, 2023 •

edited

Loading

horizonzy Mar 28, 2023 •

edited

Loading

dlg99 commented Mar 29, 2023

dlg99 Mar 29, 2023

dlg99 Mar 29, 2023

dlg99 Mar 29, 2023

dlg99 Mar 29, 2023

dlg99 Mar 29, 2023

eolivelli May 3, 2024

[Improve][Client] Group writing into the channel in PerChannelBookieClient #3886

Are you sure you want to change the base?

[Improve][Client] Group writing into the channel in PerChannelBookieClient #3886

Conversation

hangc0276 commented Mar 26, 2023

Motivation

Design

Performance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horizonzy Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

horizonzy Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

dlg99 commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

horizonzy Mar 28, 2023 •

edited

Loading

horizonzy Mar 28, 2023 •

edited

Loading