Introduce resizable inbound byte buffer #27551

Tim-Brooks · 2017-11-27T23:21:01Z

This is related to #27563. In order to interface with java nio, we must
have buffers that are compatible with ByteBuffer. This commit introduces
a basic ByteBufferReference to easily allow transferring bytes off the
wire to usage in the application.

Additionally it introduces an InboundChannelBuffer. This is a buffer
that can internally expand as more space is needed. It is designed to
be integrated with a page recycler so that it can internally reuse pages.
The final piece is moving all of the index work for writing bytes to a
channel into the WriteOperation.

Tim-Brooks · 2017-11-27T23:24:11Z

This is mostly proof of conception. I still need to add some more tests. But I wanted to see if this is more in the direction we wanted to go. @s1monw

In relation to this #27221, the InboundChannelBuffer is the piece that will be hooked up to the PageRecycler. You will see that right now when you request more bytes it is just allocating more bytes. But obviously, in the future it would reuse pages.

OutboundChannelBytes really just wraps an array of ByteBuffers and provides an index into what has been written. It does not really do anything else.

Tim-Brooks · 2017-11-28T16:09:28Z

@s1monw After thinking about this a little bit, I think that the work from OutboundChannelBytes could probably be moved to the WriteOperation. As the WriteOperation is kind of a discrete thing (it is eventually done) I think it is probably alright to keep indexes and offsets and things like that in it. Just let me know what you think.

Tim-Brooks · 2017-11-28T17:52:32Z

@s1monw I went ahead and moved all of the work related to writing bytes into the WriteOperation.

s1monw

I like where this is going, I would appreciate if you could put more inline comment etc in so it would be easier to read

s1monw · 2017-11-29T06:57:51Z

core/src/main/java/org/elasticsearch/common/bytes/ByteBufferReference.java

+
+import java.nio.ByteBuffer;
+
+public class ByteBufferReference extends BytesReference {


can we have some javadocs?

s1monw · 2017-11-29T07:06:08Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        }
+    }
+
+    public void releasePagesFromHead(int bytesToRelease) {


can we call it just release and always make it implicit that it's on the front?

s1monw · 2017-11-29T22:24:35Z

core/src/main/java/org/elasticsearch/common/bytes/ByteBufferReference.java

+    @Override
+    public BytesReference slice(int from, int length) {
+        if (from < 0 || (from + length) > this.length) {
+            throw new IllegalArgumentException("can't slice a buffer with length [" + this.length + "], with slice parameters from ["


you can rather throw and IndexOutOfBoundsException here?

s1monw · 2017-11-29T22:26:54Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+
+    public ByteBuffer[] getPreIndexBuffers() {
+        if (internalIndex == 0) {
+            return new ByteBuffer[0];


maybe assign this to a static constant?

s1monw · 2017-11-29T22:27:39Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        offset = indexInPage(bytesToRelease + offset);
+    }
+
+    public ByteBuffer[] getPreIndexBuffers() {


can you document this, I am having trouble to understand waht that means

s1monw · 2017-11-29T22:29:18Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+    private final Supplier<Page> pageSupplier;
+
+    private int capacity = 0;
+    private int internalIndex = 0;


should we rather use longs here since we can grow beyond an array an if not can we add safety guards to ensure it never does?

s1monw · 2017-11-29T22:30:06Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return buffers;
+    }
+
+    public ByteBuffer[] getPostIndexBuffers() {


same here, I need docs.

I wonder if it would be easier to have a single method that allows you to get a slice like ByteBuffer[] slice(long start, long length); then the caller can decide what is needed? this would remove the need for two methods...

s1monw · 2017-11-29T22:31:08Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return buffers;
+    }
+
+    public void incrementIndex(int delta) {


does it only go forward? should we enforce it is positive?

s1monw · 2017-11-29T22:31:54Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return capacity - internalIndex;
+    }
+
+    private int numPages(int capacity) {


up to you but maybe inline all these tiny methods? really up to you

s1monw · 2017-11-29T22:32:14Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return index & pageMask;
+    }
+
+    public static class Page implements Releasable {


is this a preparation for the future?

Yes. As a different Supplier would need access. But I'll go ahead and change it to private right now. And can adjust in whenever the "recycling" pr lands.

Tim-Brooks · 2017-11-30T02:02:19Z

@s1monw I made some changes to address your review comments.

s1monw

I did another pass but we are quite close. Looks good

s1monw · 2017-12-05T08:33:23Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+ * the pages internally. If more space is needed at the end of the buffer {@link #ensureCapacity(long)} can
+ * be called and the buffer will expand using the supplier provided.
+ */
 public class InboundChannelBuffer {


can this be final? and pkg private?

s1monw · 2017-12-05T08:38:20Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return buffers;
+    }
+
+    public ByteBuffer[] getPostIndexBuffers() {


I wonder if it would be easier to have a single method that allows you to get a slice like ByteBuffer[] slice(long start, long length); then the caller can decide what is needed? this would remove the need for two methods...

s1monw · 2017-12-05T08:39:25Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

-    private int indexInPage(int index) {
-        return index & pageMask;
+    private int indexInPage(long index) {
+        return (int) (index & pageMask);


I usually use Math.toIntExact() for this just to have extra protection but I think it's ok here

s1monw · 2017-12-05T08:41:03Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+
+    private long capacity = 0;
+    private long internalIndex = 0;
+    private int offset = 0;


this is the offset into the last buffer? can you leave a comment on it why it's an int

s1monw · 2017-12-05T08:43:53Z

test/framework/src/main/java/org/elasticsearch/transport/nio/channel/TcpReadContext.java

-        if (last == null || last.hasWriteRemaining() == false) {
-            this.references.add(NetworkBytesReference.wrap(new BytesArray(new byte[DEFAULT_READ_LENGTH])));
+        if (channelBuffer.getRemaining() == 0) {
+            channelBuffer.ensureCapacity(channelBuffer.getCapacity() + InboundChannelBuffer.PAGE_SIZE);


I am not sure if InboundChannelBuffer.PAGE_SIZE should be externally visible. Usually the contract of ensureCapacity is to guarantee some kind of capacity. it's up the the impl to ensure that we overallocate with certain page sizes. I think this call should be like this channelBuffer.ensureCapacity(channelBuffer.getCapacity() + 1) and internally we can make sure we overallocate?

then InboundChannelBuffer.PAGE_SIZE can be private

s1monw · 2017-12-05T08:45:23Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        }
+
+        int pagesToRelease = pageIndex(offset + bytesToRelease);
+        for (int i = 0; i < pagesToRelease; ++i) {


s/++i/i++

s1monw · 2017-12-05T08:45:52Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+     */
+    public ByteBuffer[] getPostIndexBuffers() {
+        if (internalIndex == capacity) {
+            return new ByteBuffer[0];


use the constant?

s1monw · 2017-12-05T08:48:00Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return (int) (index & pageMask);
+    }
+
+    private static class Page implements Releasable {


can we maybe remove this class all-together since we don't use it and can we introduce it once it's needed?

s1monw · 2017-12-05T08:48:45Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+    public static final int PAGE_SIZE = 1 << 14;
+    private static final ByteBuffer[] EMPTY_BYTE_BUFFER_ARRAY = new ByteBuffer[0];
+
+    private final int pageMask;


both pageMask and pageShift can be constants right?

s1monw · 2017-12-05T08:50:39Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+    private final int pageMask;
+    private final int pageShift;
+
+    private final ArrayDeque<Page> pages;


perfect application for an ArrayDeque!

Tim-Brooks · 2017-12-05T22:48:15Z

@s1monw I have pushed changes to address your review comments.

s1monw

LGTM left one suggestion

s1monw · 2017-12-06T06:47:45Z

test/framework/src/main/java/org/elasticsearch/transport/nio/InboundChannelBuffer.java

+        return capacity;
+    }
+
+    public long getRemaining() {


just a suggestion, can we assert that this is actually >= 0?

This is related to #27563. In order to interface with java nio, we must have buffers that are compatible with ByteBuffer. This commit introduces a basic ByteBufferReference to easily allow transferring bytes off the wire to usage in the application. Additionally it introduces an InboundChannelBuffer. This is a buffer that can internally expand as more space is needed. It is designed to be integrated with a page recycler so that it can internally reuse pages. The final piece is moving all of the index work for writing bytes to a channel into the WriteOperation.

This is a followup to elastic#27551. That commit introduced a bug where the incorrect byte buffers would be returned when we attempted a write. This commit fixes the logic.

This is a followup to #27551. That commit introduced a bug where the incorrect byte buffers would be returned when we attempted a write. This commit fixes the logic.

Tim-Brooks added 4 commits November 27, 2017 15:49

WIP

f469ff4

Merge remote-tracking branch 'upstream/master' into buffer_reuse

0cfdac9

Fix test

da086d3

Remove method

d3d9f7a

Tim-Brooks added :Distributed Coordination/Network Http and internode communication implementations >enhancement review v6.2.0 v7.0.0 labels Nov 27, 2017

Tim-Brooks requested a review from s1monw November 27, 2017 23:21

Tim-Brooks added 2 commits November 27, 2017 18:11

Add edge cases

fc626aa

Fix issue

c5f7bc3

Tim-Brooks added 2 commits November 28, 2017 10:48

Remove outbound bytes

02ac0ac

Rename method

5131f3b

Tim-Brooks changed the title ~~Introduce inbound and outbound nio bytes~~ Introduce resizable inbound byte buffer Nov 28, 2017

Cleanup channel buffer and add tests

5ff7bf5

s1monw suggested changes Nov 29, 2017

View reviewed changes

Tim-Brooks mentioned this pull request Nov 29, 2017

Implement byte array reusage in nio transport #27051

Closed

Tim-Brooks added 2 commits November 29, 2017 15:49

Merge remote-tracking branch 'upstream/master' into buffer_reuse

331d53c

Adjust for review

63dcf73

Tim-Brooks requested review from jasontedor and jaymode November 29, 2017 23:33

Add documentation

55c4da0

Merge remote-tracking branch 'upstream/master' into buffer_reuse

84ad12c

s1monw self-requested a review December 4, 2017 21:55

s1monw suggested changes Dec 5, 2017

View reviewed changes

Tim-Brooks added 4 commits December 5, 2017 10:33

Changes based on review

265a8f3

Changes based on review

62daeb5

Merge remote-tracking branch 'upstream/master' into buffer_reuse

3e9f165

Changes based on review

1678ce6

s1monw approved these changes Dec 6, 2017

View reviewed changes

Tim-Brooks added 2 commits December 6, 2017 09:52

Change from review

eaffb6f

Merge remote-tracking branch 'upstream/master' into buffer_reuse

5b0b3a4

Tim-Brooks merged commit 2aa62da into elastic:master Dec 6, 2017

Tim-Brooks mentioned this pull request Dec 7, 2017

Fix issue where the incorrect buffers are written #27695

Merged

Tim-Brooks deleted the buffer_reuse branch December 10, 2018 16:19

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		import java.nio.ByteBuffer;

		public class ByteBufferReference extends BytesReference {

Introduce resizable inbound byte buffer #27551

Introduce resizable inbound byte buffer #27551

Uh oh!

Conversation

Tim-Brooks commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tim-Brooks commented Nov 27, 2017

Uh oh!

Tim-Brooks commented Nov 28, 2017

Uh oh!

Tim-Brooks commented Nov 28, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Nov 30, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Dec 5, 2017

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Tim-Brooks commented Nov 27, 2017 •

edited

Loading