Use Netty MessageToMessage Encoder/Decoder #1034

no2chem · 2017-12-04T22:13:13Z

Overview

Description: Previously, Netty MessageToByte and ByteToMessage encoders/decoders were used to encode/decode Corfu messages. Since our messages are framed by the LengthField encoder/decoder, there is no need for extra state logic to determine how many messages there are, and this enables these channels to be implemented as sharable, which should reduce overhead in the Netty pipeline.

Why should this be merged: Relatively simple and low risk change to the Netty pipeline which should reduce overhead and slightly improve performance.

Checklist (Definition of Done):

There are no TODOs left in the code
Coding conventions (e.g. for logging, unit tests) have been followed
Change is covered by automated tests
Public API has Javadoc

coveralls · 2017-12-04T22:38:05Z

Coverage decreased (-0.003%) to 70.599% when pulling d7387d9 on MsgToMsgM into cbb2c2a on master.

coveralls · 2017-12-05T00:45:55Z

Coverage increased (+0.007%) to 70.609% when pulling 89877f4 on MsgToMsgM into cbb2c2a on master.

corfudb-performance · 2017-12-05T05:37:58Z

Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 89877f4.

*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 10 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 10 threads, Disk mode

An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at:
Pull Request #1034 Graphs

annym · 2017-12-07T02:32:17Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

 import lombok.extern.slf4j.Slf4j;


 /**
 * Created by mwei on 10/1/15.
 */
 @Slf4j
-public class NettyCorfuMessageDecoder extends ByteToMessageDecoder {
+@Sharable


I believe It is not actually enough just to annotate it as 'sharable' (this is only an informational annotation). This would require CorfuServer and NettyClientRouter to add the same instance to the pipeline. Maybe this can explain why we don't actually see improvements in the performance runs.

In the Documentation of the annotation you can find this description:

This annotation is provided for documentation purpose, just like the JCIP annotations.

For more details you can read the "Shared and Exclusive Channel Handlers" of this blog

@annym Thanks! I made the change to keep the same encoder/decoder. I wouldn't expect a really significant performance increase until we start using multiple channels.

Great! You are right we would see improvements only when using multiple channels.

coveralls · 2017-12-07T03:33:08Z

Coverage increased (+0.1%) to 70.706% when pulling 1fbb7bc on MsgToMsgM into cbb2c2a on master.

corfudb-performance · 2017-12-07T05:38:23Z

Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 1fbb7bc.

*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 10 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 10 threads, Disk mode

An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at:
Pull Request #1034 Graphs

annym · 2017-12-07T19:37:24Z

Can you please explain what you mean with "Since our messages are framed by the LengthField encoder/decoder"? Not sure I understand what this means. Because if so, shouldn't we extend from LengthFieldBasedFrameDecoder

no2chem · 2017-12-07T22:40:22Z

@annym Great point, not sure why it wasn't like this to begin with. I updated the decoder to extend from LengthFieldBasedFrameDecoder per your suggestions. I'll get to modifying the encoder next. Unfortunately they can no longer be sharable (since the LengthFieldBasedFrameDecoder holds state), but I think having the pipeline shorter is much better!

corfu-repo · 2017-12-07T23:01:58Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

@@ -2,36 +2,34 @@

 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;


Remove this unused import 'io.netty.buffer.Unpooled'.

corfu-repo · 2017-12-07T23:01:58Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

@@ -2,36 +2,34 @@

 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import io.netty.channel.ChannelHandler.Sharable;


Remove this unused import 'io.netty.channel.ChannelHandler.Sharable'.

corfu-repo · 2017-12-07T23:01:59Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandler;


Remove this unused import 'io.netty.channel.ChannelInboundHandler'.

corfu-repo · 2017-12-07T23:01:59Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelInboundHandler;
 import io.netty.handler.codec.ByteToMessageDecoder;


Remove this unused import 'io.netty.handler.codec.ByteToMessageDecoder'.

corfu-repo · 2017-12-07T23:02:00Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

 import io.netty.handler.codec.ByteToMessageDecoder;

+import io.netty.handler.codec.LengthFieldBasedFrameDecoder;
+import io.netty.handler.codec.MessageToMessageDecoder;


Remove this unused import 'io.netty.handler.codec.MessageToMessageDecoder'.

corfu-repo · 2017-12-07T23:02:00Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

 import io.netty.handler.codec.ByteToMessageDecoder;

+import io.netty.handler.codec.LengthFieldBasedFrameDecoder;
+import io.netty.handler.codec.MessageToMessageDecoder;
 import java.util.List;


Remove this unused import 'java.util.List'.

corfu-repo · 2017-12-07T23:02:01Z

runtime/src/main/java/org/corfudb/protocols/wireprotocol/NettyCorfuMessageDecoder.java

 import java.util.List;

+import javax.annotation.Nonnull;


Remove this unused import 'javax.annotation.Nonnull'.

corfu-repo · 2017-12-07T23:02:02Z

SonarQube analysis reported 9 issues

9 minor

Watch the comments in this conversation to review them.

2 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

CorfuServer.java#L46: Remove this unused import 'org.corfudb.security.tls.TlsUtils'.
NettyClientRouter.java#L53: Remove this unused import 'org.corfudb.security.tls.TlsUtils'.

coveralls · 2017-12-07T23:02:30Z

Coverage increased (+0.1%) to 70.722% when pulling b822f38 on MsgToMsgM into cbb2c2a on master.

corfudb-performance · 2017-12-08T02:36:29Z

Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit b822f38.

*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 10 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 10 threads, Disk mode

An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at:
Pull Request #1034 Graphs

no2chem added 2 commits December 4, 2017 14:06

Convert to MessageToMessageEncoder/Decoder

caba241

optimize imports

d7387d9

no2chem added the typ: performance label Dec 4, 2017

no2chem added this to the 0.2.0 milestone Dec 4, 2017

no2chem self-assigned this Dec 4, 2017

no2chem requested review from Maithem, medhavidhawan and annym December 4, 2017 22:13

Add javadoc

89877f4

annym requested changes Dec 7, 2017

View reviewed changes

Keep encoders/decoders across channels

1fbb7bc

extend LengthFieldBasedFrameDecoder

b822f38

corfu-repo reviewed Dec 7, 2017

View reviewed changes

Rtremblay removed this from the 0.2.0 milestone Dec 20, 2017

Rtremblay added this to the 0.4.0 milestone Dec 20, 2017

no2chem closed this Jun 18, 2018

Maithem deleted the MsgToMsgM branch July 29, 2019 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Netty MessageToMessage Encoder/Decoder #1034

Use Netty MessageToMessage Encoder/Decoder #1034

no2chem commented Dec 4, 2017 •

edited

Loading

coveralls commented Dec 4, 2017

coveralls commented Dec 5, 2017

corfudb-performance commented Dec 5, 2017

annym Dec 7, 2017

no2chem Dec 7, 2017

annym Dec 7, 2017

coveralls commented Dec 7, 2017

corfudb-performance commented Dec 7, 2017

annym commented Dec 7, 2017

no2chem commented Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo Dec 7, 2017

corfu-repo commented Dec 7, 2017

coveralls commented Dec 7, 2017

corfudb-performance commented Dec 8, 2017

		@@ -2,36 +2,34 @@

		import io.netty.buffer.ByteBuf;
		import io.netty.buffer.Unpooled;

		import io.netty.channel.ChannelHandlerContext;
		import io.netty.channel.ChannelInboundHandler;

Use Netty MessageToMessage Encoder/Decoder #1034

Use Netty MessageToMessage Encoder/Decoder #1034

Conversation

no2chem commented Dec 4, 2017 • edited Loading

Overview

Checklist (Definition of Done):

coveralls commented Dec 4, 2017

coveralls commented Dec 5, 2017

corfudb-performance commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 7, 2017

corfudb-performance commented Dec 7, 2017

annym commented Dec 7, 2017

no2chem commented Dec 7, 2017

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

corfu-repo commented Dec 7, 2017

2 extra issues

coveralls commented Dec 7, 2017

corfudb-performance commented Dec 8, 2017

no2chem commented Dec 4, 2017 •

edited

Loading