Implement byte array reusage in nio transport #27051

Tim-Brooks · 2017-10-19T16:09:27Z

This commit implements a manner of reusing byte arrays when reading from
a nio channel. Specifically, it adds the ability to request a single
byte page from a BigArrays instance. When a byte page is requested, it
is added to a ChannelBuffer. Internally the byte pages are reference
counted to handle the case where a message causes a byte page to be
sliced in half.

Introducing new concepts (ChannelBuffer, NetworkBytesReference,
NetworkBytes) was necessary as our existing implementations
(BigByteArray, PagedBytesReference, various streams) do not interface
well with java nio.

Tim-Brooks · 2017-10-19T16:11:35Z

This still needs some more granular testing. And some settings to disable byte array reuse.

But I wanted to push it up here as a POC and get some feedback and see what others think about this approach before I continue to do more work.

Tim-Brooks · 2017-10-20T14:46:21Z

This change has difficulties working with the test harness currently as we check breakers are at zero and pages have been released between each test. In this PR, pages can still be attached to active channels that live between tests causing assertions to fail.

To work around, I disabled circuit breaking for channel buffers (as I'm not sure we would currently want that anyway). And I temporarily disabled checking pages released between tests (only afterClass). I made that change as I am interested in seeing a CI run without these failures. But that would probably not be the permanent solution.

s1monw

I did a first pass. I like where it's going. Thanks of the iterations after our chat. very helpful

s1monw · 2017-10-25T08:42:27Z

core/src/main/java/org/elasticsearch/common/collect/IndexedArrayDeque.java

+    @Override
+    protected void ensureBufferSpace(int expectedAdditions) {
+        try {
+            super.ensureBufferSpace(expectedAdditions);


oh man that is terrible. We should contribute this back to hppc. Can you make sure we do this? I don't think it should ever catch OOM

oh and, good catch!

s1monw · 2017-10-25T08:43:44Z

core/src/main/java/org/elasticsearch/common/util/BytesPage.java

+
+    @Override
+    public void close() {
+        if (recycler != null) {


Use IOUtils#close

s1monw · 2017-10-27T08:07:06Z

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

+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public class BytesPage extends BytesArray implements Releasable {


I think we can move this all into BytesArray instead? I don't think we need another class. just make the recycler optional as you did and we are good?

s1monw · 2017-10-27T08:26:32Z

core/src/main/java/org/elasticsearch/common/util/BigArrays.java


+    /**
+     * Allocate a new {@link BytesPage}.
+     */


Now I am starting to wrap my head around this and I think I have an idea of how we can simplify this more. We are currently using this concrete BytesPage instead of org.elasticsearch.common.util.ByteArray which is due to the lack of accessing the underlying bytes. Yet, using ByteArray might have some beauty to it, especially since it allows to have a single instance that we can simply resize by calling BigArrays#resize instead of doing all the accounting ourself. So in-order to do that I think we would need to add a couple of methods to ByteArray for instance this:

public interface ByteArray extends BigArray { //... all the existing methods // this sets the page for the given offset to the bytes ref without materializing it // we can then in-turn wrap this with a ByteBuffer to read from NIO public void getPageReference(int offset, BytesRef ref); // releases all allocated byte pages until the given offset // the bytes at the given offset are not accessible at offset `0` public void shrinkTo(int offset); // this returns a bytes reference to the given offset with the given length without releasing etc. public BytesReference toBytesReference(int offset, int length); }

this way I think we can keep all the logic of releaseing and advancing to the ByteArray utils and we are basically just moving along with the messages. I think this would make a lot of code much simpler in here. WDYT?

I'm not really opposed to modifying ByteArray to fit our needs. I even considered using it when I first worked on this PR but I did not want to do much modification to it at the time, so I held off.

I think in regards to:

public BytesReference toBytesReference(int offset, int length);

Both of those pieces of functionality should be added pretty trivially using PagedBytesReference.

I don't really think public void getPageReference(int offset, BytesRef ref) is needed as PagedBytesReference returns a BytesRef iterator that will not materialize bytes. And most nio apis support vectorized operations.

shrinkTo is more complicated as it does not look like the ByteArray was designed to be shrunk. Nor is it a ring buffer which means that every time you slice a message off the head (and then shrink) you have to rearrange the stuff in the arrays.

s1monw · 2017-10-27T08:33:40Z

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

+    @Override
+    public void close() {
+        if (closed == false) {
+            channelBuffer.close();


maybe just protected channelbuffer from having sideeffects of multiple closing and remove the closed flag? it seems unused?

s1monw · 2017-10-27T08:33:44Z

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

        try {
            headOp.flush();
        } catch (IOException e) {
            headOp.getListener().onFailure(e);


maybe do the close in a finally if headOp.getListener().onFailure(e); barfs?

s1monw · 2017-10-27T08:33:45Z

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


        if (headOp.isFullyFlushed()) {
            headOp.getListener().onResponse(channel);
+            headOp.close();


s1monw · 2017-10-27T08:33:48Z

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


            try {
-                BytesReference messageWithoutHeader = message.slice(6, message.length() - 6);
+                BytesReference messageWithoutHeader = messageBytes.slice(6, messageBytes.length() - 6);


maybe move the two lines you access the message into the try block

s1monw · 2017-10-27T08:33:57Z

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

-import java.nio.ByteBuffer;
-import java.util.Iterator;
-
-public class NetworkBytesReference extends BytesReference {


s1monw · 2017-10-27T08:34:03Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

-    protected static void checkStaticState() throws Exception {
-        MockPageCacheRecycler.ensureAllPagesAreReleased();
+    protected static void checkStaticState(boolean afterClass) throws Exception {
+        if (afterClass) {


do we need this in general or can we maybe only do this in specific test cases?

I'm not sure I view this as a long term fix. I just wanted to full CI run without failures caused by holding bytes in between tests. We maybe should talk about the best way to deal with this.

ok understood. maybe mark it with //nocommit

s1monw · 2017-11-01T14:56:53Z

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


+    @Override
+    public void close() {
+        // TODO: Do we want to throw an exception to expose misuse?


the closeable contract says it's closeable multple times so we are good I guess.

Tim-Brooks · 2017-11-03T15:22:29Z

I'm working on another PR related to integrating this with our recyclers. I don't need any reviews for this PR until that work is integrated.

Tim-Brooks · 2017-11-29T22:46:34Z

I am closing this PR. We have refined the data structures a bit in #27551 and will eventually build something off that.

Tim-Brooks added 6 commits October 19, 2017 00:14

WIP

b76039b

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

5c4f9c2

Transition to network bytes for writes

198cd51

Use channel buffer on incoming channels

6294630

Use actual big arrays

9496186

Close read context

358983e

Tim-Brooks added :Distributed Coordination/Network Http and internode communication implementations >non-issue review v6.1.0 v7.0.0 labels Oct 19, 2017

Tim-Brooks requested review from jasontedor, jaymode and s1monw October 19, 2017 16:09

Tim-Brooks added 2 commits October 19, 2017 18:17

A few cleanups

0fdf0be

Do not circuit break and only assert pages released after class

f447893

Tim-Brooks added 12 commits October 20, 2017 17:44

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

64ce813

Improve tests

f197df7

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

636d504

Fix edge case with slicing while using channel buffer

d28cc9d

Make sure to close removed buffer

fadaf73

Implement vectorized reads

4d7dd1b

WIP

962e493

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

7844f5e

Merge branch 'reuse_buffers' into simplify_for_simon

4c542cc

WIP

fb6c81e

WIP

95f2ff1

Fix tests

498e8e0

A few cleanups

caea6d9

s1monw reviewed Oct 27, 2017

View reviewed changes

Tim-Brooks added 3 commits October 27, 2017 09:46

Small fix related to channel buffer

1168d9b

Changes based on review

1161c83

Changes based on review

af21323

s1monw reviewed Nov 1, 2017

View reviewed changes

Tim-Brooks mentioned this pull request Nov 2, 2017

Add method to drop bytes from head of a BigArray #27221

Closed

Tim-Brooks mentioned this pull request Nov 3, 2017

Introduce new transports driven by java.nio #27260

Closed

12 tasks

Tim-Brooks closed this Nov 29, 2017

Tim-Brooks deleted the reuse_buffers branch November 14, 2018 14:50

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


		import java.util.concurrent.atomic.AtomicBoolean;

		public class BytesPage extends BytesArray implements Releasable {

Implement byte array reusage in nio transport #27051

Implement byte array reusage in nio transport #27051

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Tim-Brooks commented Oct 19, 2017

Uh oh!

Tim-Brooks commented Oct 20, 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!

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 3, 2017

Uh oh!

Tim-Brooks commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tim-Brooks commented Oct 19, 2017 •

edited

Loading