maxBytesPerRead channel configuration #3885

Scottmitch · 2015-06-12T20:24:44Z

Motiviation:
The current read loops don't fascilitate reading a maximum amount of bytes. This capability is useful to have more fine grain control over how much data is injested.

Modifications:

Add a setMaxBytesPerRead(int) and getMaxBytesPerRead() to ChannelConfig
Add a setMaxBytesPerIndividualRead(int) and getMaxBytesPerIndividualRead to ChannelConfig
Add methods to RecvByteBufAllocator so that a pluggable scheme can be used to control the behavior of the read loop.
Modify read loop for all transport types to respect the new RecvByteBufAllocator API

Result:
The ability to control how many bytes are read for each read operation/loop, and a more extensible read loop.

Scottmitch · 2015-06-12T20:25:48Z

@trustin - Would you mind reviewing this one?

Scottmitch · 2015-06-12T23:16:53Z

transport/src/main/java/io/netty/channel/DefaultChannelConfig.java

+        } else if (option == MAX_BYTES_PER_READ_PAIR) {
+            @SuppressWarnings("unchecked")
+            Entry<Integer, Integer> pair = (Entry<Integer, Integer>) value;
+            setMaxBytesPerReadPair(pair.getKey(), pair.getValue());


I think something similar can be done to resolve #3806.

trustin · 2015-06-15T01:13:17Z

codec-dns/src/test/java/io/netty/handler/codec/dns/package-info.java

- * A DNS client that queries a server and checks if query information and
- * responses are valid to ensure codec is correct.
- */
-package io.netty.handler.codec.dns;


Maybe doesn't belong in this PR, but this is a duplicate of the package-info.java in the main directory tree. Was (and has been) causing an issue in eclipse IDE for quite some time now...resolved to make "real" errors I introduced during interface updates easier to see.

Ah, nevermind, then.

trustin · 2015-06-15T01:16:29Z

@Scottmitch Sorry for a delay. Will review some time this week.

Scottmitch · 2015-06-15T16:30:31Z

@netkins build

Scottmitch · 2015-06-23T16:33:14Z

@trustin - Bump.

trustin · 2015-06-30T09:48:35Z

Do the new configuration properties work only when the RecvByteBufAllocator is aware of them? If so, why don't we move them to the RecvByteBufAllocator implementation?

Scottmitch · 2015-06-30T23:44:02Z

Yes. Currently MaxBytesRecvByteBufAllocator is the only consumer of the new properties. The RecvByteBufAllocator implementations have also encapsulated the responsibility of the existing getMaxMessagesPerRead(). I think if we move the new parameters it may make sense to also move getMaxMessagesPerRead() into the respective implementation that uses it (this change is only for 4.1 and 5.0). WDYT?

Scottmitch · 2015-06-30T23:54:10Z

@trustin - Another thing to consider is moving these configuration options from ChannelConfig to an implementation of RecvByteBufAllocator may make it a bit more cumbersome to change on the fly. For example this may require conditionals based upon the run time type of the RecvByteBufAllocator. For example:

RecvByteBufAllocator recvAlloc = channel.config().getRecvByteBufAllocator();
if (recvAlloc instanceof MaxBytesRecvByteBufAllocator) {
  MaxBytesRecvByteBufAllocator maxRecvAlloc = (MaxBytesRecvByteBufAllocator) recvAlloc;
  maxRecvAlloc.setMaxMessagesPerRead(...);
  maxRecvAlloc.setMaxBytesPerIndividualRead(...);
} else if (recvAlloc instanceof ...) {
}

At least you can be sure what your are setting will actually be respected though...

trustin · 2015-07-01T00:09:14Z

I think if we move the new parameters it may make sense to also move getMaxMessagesPerRead() into the respective implementation that uses it (this change is only for 4.1 and 5.0). WDYT?

Sounds good to me!

Another thing to consider is moving these configuration options from ChannelConfig to an implementation of RecvByteBufAllocator may make it a bit more cumbersome to change on the fly.

Yeah, how about using the type parameter for the return type for loose typing to lessen the burden?

interface ChannelConfig {
    <T extends RecvByteBufAllocator> T getRecvByteBufAllocator();
}

MaxBytesRecvByteBufAllocator alloc = ch.config().getRecvByteBufAllocator();
...

This won't work when a user is not sure about the type of the current RecvByteBufAllocator, though...

Scottmitch · 2015-07-01T00:16:58Z

@trustin - Thanks for the feedback. I will move forward with these updates and ping you when ready.

Scottmitch · 2015-07-01T18:58:39Z

transport-sctp/src/main/java/io/netty/channel/sctp/oio/OioSctpChannel.java

@@ -184,8 +185,8 @@ protected int doReadMessages(List<Object> msgs) throws Exception {

        Set<SelectionKey> reableKeys = readSelector.selectedKeys();
        try {
-            for (SelectionKey ignored : reableKeys) {
-                RecvByteBufAllocator.Handle allocHandle = unsafe().recvBufAllocHandle();
+            for (@SuppressWarnings("unused") SelectionKey ignored : reableKeys) {


Not sure why this loop is necessary...see #3884

I'm not sure, either. Could you explain, @jestan ?

Scottmitch · 2015-07-01T19:01:05Z

@trustin @normanmaurer - PTAL.

trustin · 2015-07-07T06:07:02Z

@Scottmitch LGTM. @normanmaurer, any comments?

normanmaurer · 2015-07-07T07:53:04Z

transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollDomainSocketChannelConfig.java

@@ -58,11 +58,6 @@

        return true;
    }
-    @Override
-    public EpollDomainSocketChannelConfig setMaxMessagesPerRead(int maxMessagesPerRead) {


I'm right that we only want this for 4.1 and master ?

Yes. #3885 (comment).

I'm still not 100 % convinced that we can do this change in 4.1 in terms of API breakage. @trustin WDYT ?

@normanmaurer Yeah, we can't just remove it. How about deprecating it and delegate to Max*Allocator if the current recv allocator is of compatible type, or throw an exception if it's incompatible?

@trustin @normanmaurer - Done.

normanmaurer · 2015-07-07T08:00:13Z

@Scottmitch a few more comments and questions. Good work!

Scottmitch · 2015-07-16T00:39:27Z

@trustin - I would like to merge this but there are just a few followup discussion items above. Would you mind weighing in?

Scottmitch · 2015-08-04T19:09:47Z

@trustin @normanmaurer - PTAL.

Scottmitch · 2015-08-04T19:24:08Z

transport/src/main/java/io/netty/channel/DefaultChannelConfig.java

+            if (metadata == null) {
+                throw new NullPointerException("metadata");
+            }
+            MaxMessagesRecvByteBufAllocator maxMsgAllocator = (MaxMessagesRecvByteBufAllocator) allocator;


@trustin - This is how I am preserving the 16 max messages for ServerSockets/NioByteStream and 1 max message otherwise. WDYT?

LGTM. Thank you for your patience.

Scottmitch · 2015-08-06T05:58:19Z

@trustin - ~~I guess now that this doesn't break any APIs we could also consider putting this in 4.0. It would be nice to be consistent between 4.0 and 4.1...WDYT?~~

Nvm...forgot about changes to RecvByteBufAllocator.Handle

Motiviation: The current read loops don't fascilitate reading a maximum amount of bytes. This capability is useful to have more fine grain control over how much data is injested. Modifications: - Add a setMaxBytesPerRead(int) and getMaxBytesPerRead() to ChannelConfig - Add a setMaxBytesPerIndividualRead(int) and getMaxBytesPerIndividualRead to ChannelConfig - Add methods to RecvByteBufAllocator so that a pluggable scheme can be used to control the behavior of the read loop. - Modify read loop for all transport types to respect the new RecvByteBufAllocator API Result: The ability to control how many bytes are read for each read operation/loop, and a more extensible read loop.

Scottmitch · 2015-08-06T06:40:32Z

Ran some quick perf tests on the before and after for this PR just to make sure there was no regression, and things look good (using NIO):

before

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    22.07ms   33.58ms   1.11s    85.72%
    Req/Sec   132.23k    33.88k  241.31k    66.05%
  251377804 requests in 2.00m, 33.24GB read
Requests/sec: 2093223.99
Transfer/sec:    283.47MB

this PR

  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    21.36ms   34.33ms   1.28s    85.67%
    Req/Sec   136.39k    29.18k  231.36k    67.75%
  259304381 requests in 2.00m, 34.29GB read
Requests/sec: 2159239.24
Transfer/sec:    292.41MB

Scottmitch · 2015-08-06T07:00:55Z

Cherry-picked 4.1 (cf171ff) master (a48e5c7)

trustin · 2015-08-06T09:41:10Z

👍

Scottmitch self-assigned this Jun 12, 2015

Scottmitch modified the milestones: 4.1.0.Final, 4.1.0.Beta6 Jun 12, 2015

Scottmitch added the feature label Jun 12, 2015

Scottmitch force-pushed the read_max_bytes branch 5 times, most recently from 1bd0235 to e818942 Compare June 12, 2015 23:13

Scottmitch reviewed Jun 12, 2015
View reviewed changes

Scottmitch force-pushed the read_max_bytes branch from e818942 to f18f70a Compare June 13, 2015 20:46

trustin reviewed Jun 15, 2015
View reviewed changes

Scottmitch force-pushed the read_max_bytes branch 2 times, most recently from a22a712 to c184d89 Compare June 30, 2015 23:27

Scottmitch force-pushed the read_max_bytes branch 2 times, most recently from 6b8da06 to ebcdd88 Compare July 1, 2015 18:55

Scottmitch reviewed Jul 1, 2015
View reviewed changes

Scottmitch force-pushed the read_max_bytes branch from ebcdd88 to 84b48da Compare July 1, 2015 19:00

Scottmitch force-pushed the read_max_bytes branch from 84b48da to ac6ae58 Compare July 2, 2015 17:38

normanmaurer reviewed Jul 7, 2015
View reviewed changes

Scottmitch force-pushed the read_max_bytes branch from ac6ae58 to 763639f Compare July 8, 2015 19:45

Scottmitch force-pushed the read_max_bytes branch from a4576f8 to cbdc931 Compare August 4, 2015 19:09

Scottmitch force-pushed the read_max_bytes branch from cbdc931 to d97560e Compare August 4, 2015 19:22

Scottmitch reviewed Aug 4, 2015
View reviewed changes

Scottmitch force-pushed the read_max_bytes branch from d97560e to 749f614 Compare August 4, 2015 21:03

Scottmitch force-pushed the read_max_bytes branch from 749f614 to f473dcb Compare August 6, 2015 06:12

Scottmitch closed this Aug 6, 2015

Scottmitch deleted the read_max_bytes branch August 6, 2015 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maxBytesPerRead channel configuration #3885

maxBytesPerRead channel configuration #3885

Scottmitch commented Jun 12, 2015

Scottmitch commented Jun 12, 2015

Scottmitch Jun 12, 2015

trustin Jun 15, 2015

Scottmitch Jun 15, 2015

trustin Jun 30, 2015

trustin commented Jun 15, 2015

Scottmitch commented Jun 15, 2015

Scottmitch commented Jun 23, 2015

trustin commented Jun 30, 2015

Scottmitch commented Jun 30, 2015

Scottmitch commented Jun 30, 2015

trustin commented Jul 1, 2015

Scottmitch commented Jul 1, 2015

Scottmitch Jul 1, 2015

trustin Jul 7, 2015

Scottmitch commented Jul 1, 2015

trustin commented Jul 7, 2015

normanmaurer Jul 7, 2015

Scottmitch Jul 7, 2015

normanmaurer Jul 8, 2015

trustin Jul 30, 2015

Scottmitch Aug 4, 2015

normanmaurer commented Jul 7, 2015

Scottmitch commented Jul 16, 2015

Scottmitch commented Aug 4, 2015

Scottmitch Aug 4, 2015

trustin Aug 6, 2015

Scottmitch commented Aug 6, 2015

Scottmitch commented Aug 6, 2015

Scottmitch commented Aug 6, 2015

trustin commented Aug 6, 2015

maxBytesPerRead channel configuration #3885

maxBytesPerRead channel configuration #3885

Conversation

Scottmitch commented Jun 12, 2015

Scottmitch commented Jun 12, 2015

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

trustin commented Jun 15, 2015

Scottmitch commented Jun 15, 2015

Scottmitch commented Jun 23, 2015

trustin commented Jun 30, 2015

Scottmitch commented Jun 30, 2015

Scottmitch commented Jun 30, 2015

trustin commented Jul 1, 2015

Scottmitch commented Jul 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Jul 1, 2015

trustin commented Jul 7, 2015

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

normanmaurer commented Jul 7, 2015

Scottmitch commented Jul 16, 2015

Scottmitch commented Aug 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Aug 6, 2015

Scottmitch commented Aug 6, 2015

Scottmitch commented Aug 6, 2015

trustin commented Aug 6, 2015