Review comments

NiteshKant · Sep 16, 2021 · d0cbce1 · d0cbce1
1 parent 819bba6
commit d0cbce1
Show file tree

Hide file tree

Showing 7 changed files with 150 additions and 214 deletions.
diff --git a/buffer/src/main/java/io/netty/buffer/api/Buffer.java b/buffer/src/main/java/io/netty/buffer/api/Buffer.java
@@ -15,6 +15,8 @@
  */
 package io.netty.buffer.api;
 
+import io.netty.buffer.api.internal.Statics;
+
 import java.nio.ByteBuffer;
 import java.nio.charset.Charset;
 
@@ -127,7 +129,7 @@ public interface Buffer extends Resource<Buffer>, BufferAccessor {
      * {@link #writerOffset()}.
      * @throws BufferClosedException if this buffer is closed.
      */
-    default Buffer accumulateReaderOffset(int delta) {
+    default Buffer skipReadable(int delta) {
         return readerOffset(readerOffset() + delta);
     }
 
@@ -159,7 +161,7 @@ default Buffer accumulateReaderOffset(int delta) {
      * @throws BufferClosedException if this buffer is closed.
      * @throws BufferReadOnlyException if this buffer is {@linkplain #readOnly() read-only}.
      */
-    default Buffer accumulateWriterOffset(int delta) {
+    default Buffer skipWritable(int delta) {
         return writerOffset(writerOffset() + delta);
     }
 
@@ -289,7 +291,7 @@ default int writableBytes() {
      * @return This buffer.
      */
     default Buffer writeCharSequence(CharSequence source, Charset charset) {
-        BufferUtils.writeCharSequence(source, this, charset);
+        Statics.writeCharSequence(source, this, charset);
         return this;
     }
 
@@ -304,7 +306,7 @@ default Buffer writeCharSequence(CharSequence source, Charset charset) {
      * this buffer.
      */
     default CharSequence readCharSequence(int length, Charset charset) {
-        return BufferUtils.readCharSequence(this, length, charset);
+        return Statics.readCharSequence(this, length, charset);
     }
 
     /**
@@ -538,7 +540,7 @@ default Buffer copy() {
     Buffer copy(int offset, int length);
 
     /**
-     * Splits the buffer into two, at the {@code offset} from current {@linkplain #readerOffset()} reader offset}
+     * Splits the buffer into two, at the {@code offset} from the current {@linkplain #readerOffset()} reader offset}
      * position.
      * <p>
      * The region of this buffer that contain the previously read and readable bytes till the {@code offset}, will be
@@ -589,7 +591,7 @@ default Buffer readSplit(int offset) {
     }
 
     /**
-     * Splits the buffer into two, at the {@code offset} from current {@linkplain #writerOffset()} writer offset}
+     * Splits the buffer into two, at the {@code offset} from the current {@linkplain #writerOffset()} writer offset}
      * position.
      * <p>
      * The region of this buffer that contain the previously read and readable bytes till the {@code offset}, will be

diff --git a/buffer/src/main/java/io/netty/buffer/api/BufferUtils.java b/buffer/src/main/java/io/netty/buffer/api/BufferUtils.java
diff --git a/buffer/src/main/java/io/netty/buffer/api/internal/Statics.java b/buffer/src/main/java/io/netty/buffer/api/internal/Statics.java
@@ -19,6 +19,7 @@
 import io.netty.buffer.api.BufferClosedException;
 import io.netty.buffer.api.BufferReadOnlyException;
 import io.netty.buffer.api.Drop;
+import io.netty.util.AsciiString;
 
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
@@ -27,8 +28,11 @@
 import java.lang.invoke.VarHandle;
 import java.lang.ref.Cleaner;
 import java.nio.ByteBuffer;
+import java.nio.charset.Charset;
 import java.util.concurrent.atomic.LongAdder;
 
+import static io.netty.util.CharsetUtil.US_ASCII;
+
 public interface Statics {
     LongAdder MEM_USAGE_NATIVE = new LongAdder();
     Cleaner CLEANER = Cleaner.create();
@@ -211,4 +215,25 @@ static int countBorrows(ResourceSupport<?, ?> obj) {
     static void unsafeSetDrop(ResourceSupport<?, ?> obj, Drop<?> replacement) {
         obj.unsafeSetDrop((Drop) replacement);
     }
+
+    static CharSequence readCharSequence(Buffer source, int length, Charset charset) {
+        byte[] data = new byte[length];
+        source.copyInto(source.readerOffset(), data, 0, length);
+        source.skipReadable(length);
+        if (US_ASCII.equals(charset)) {
+            return new AsciiString(data);
+        }
+        return new String(data, 0, length, charset);
+    }
+
+    static void writeCharSequence(CharSequence source, Buffer destination, Charset charset) {
+        if (US_ASCII.equals(charset) && source instanceof AsciiString) {
+            AsciiString asciiString = (AsciiString) source;
+            destination.writeBytes(asciiString.array(), asciiString.arrayOffset(), source.length());
+            return;
+        }
+        // TODO: Copy optimized writes from ByteBufUtil
+        byte[] bytes = source.toString().getBytes(charset);
+        destination.writeBytes(bytes);
+    }
 }
diff --git a/buffer/src/test/java/io/netty/buffer/api/tests/BufferCharSequenceTest.java b/buffer/src/test/java/io/netty/buffer/api/tests/BufferCharSequenceTest.java
@@ -32,54 +32,48 @@ public class BufferCharSequenceTest extends BufferTestSupport {
     @ParameterizedTest
     @MethodSource("allocators")
     void readCharSequence(Fixture fixture) {
-        try (BufferAllocator allocator = fixture.createAllocator()) {
-            try (Buffer buf = allocator.allocate(32)) {
-                String data = "Hello World";
-                buf.writeBytes(data.getBytes());
-                assertEquals(data.length(), buf.writerOffset());
-                assertEquals(0, buf.readerOffset());
+        try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(32)) {
+            String data = "Hello World";
+            buf.writeBytes(data.getBytes(US_ASCII));
+            assertEquals(data.length(), buf.writerOffset());
+            assertEquals(0, buf.readerOffset());
 
-                final CharSequence charSequence = buf.readCharSequence(data.length(), US_ASCII);
-                Assertions.assertEquals(data, charSequence.toString());
-                assertEquals(data.length(), buf.writerOffset());
-                assertEquals(data.length(), buf.readerOffset());
-            }
+            final CharSequence charSequence = buf.readCharSequence(data.length(), US_ASCII);
+            Assertions.assertEquals(data, charSequence.toString());
+            assertEquals(data.length(), buf.writerOffset());
+            assertEquals(data.length(), buf.readerOffset());
         }
     }
 
     @ParameterizedTest
     @MethodSource("allocators")
     void writeCharSequence(Fixture fixture) {
-        try (BufferAllocator allocator = fixture.createAllocator()) {
-            try (Buffer buf = allocator.allocate(32)) {
-                AsciiString data = new AsciiString("Hello world".getBytes(US_ASCII));
-                buf.writeCharSequence(data, US_ASCII);
-                assertEquals(data.length(), buf.writerOffset());
-                assertEquals(0, buf.readerOffset());
+        try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(32)) {
+            AsciiString data = new AsciiString("Hello world".getBytes(US_ASCII));
+            buf.writeCharSequence(data, US_ASCII);
+            assertEquals(data.length(), buf.writerOffset());
+            assertEquals(0, buf.readerOffset());
 
-                final byte[] read = readByteArray(buf);
-                Assertions.assertEquals(data.toString(), new String(read, US_ASCII));
-                assertEquals(data.length(), buf.writerOffset());
-                assertEquals(data.length(), buf.readerOffset());
-            }
+            final byte[] read = readByteArray(buf);
+            Assertions.assertEquals(data.toString(), new String(read, US_ASCII));
+            assertEquals(data.length(), buf.writerOffset());
+            assertEquals(data.length(), buf.readerOffset());
         }
     }
 
     @ParameterizedTest
     @MethodSource("allocators")
     void readAndWriteCharSequence(Fixture fixture) {
-        try (BufferAllocator allocator = fixture.createAllocator()) {
-            try (Buffer buf = allocator.allocate(32)) {
-                AsciiString data = new AsciiString("Hello world".getBytes(US_ASCII));
-                buf.writeCharSequence(data, US_ASCII);
-                assertEquals(data.length(), buf.writerOffset());
-                assertEquals(0, buf.readerOffset());
+        try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(32)) {
+            AsciiString data = new AsciiString("Hello world".getBytes(US_ASCII));
+            buf.writeCharSequence(data, US_ASCII);
+            assertEquals(data.length(), buf.writerOffset());
+            assertEquals(0, buf.readerOffset());
 
-                final CharSequence read = buf.readCharSequence(data.length(), US_ASCII);
-                Assertions.assertEquals(data, read);
-                assertEquals(data.length(), buf.writerOffset());
-                assertEquals(data.length(), buf.readerOffset());
-            }
+            final CharSequence read = buf.readCharSequence(data.length(), US_ASCII);
+            Assertions.assertEquals(data, read);
+            assertEquals(data.length(), buf.writerOffset());
+            assertEquals(data.length(), buf.readerOffset());
         }
     }
 }
diff --git a/buffer/src/test/java/io/netty/buffer/api/tests/BufferOffsetTest.java b/buffer/src/test/java/io/netty/buffer/api/tests/BufferOffsetTest.java
diff --git a/buffer/src/test/java/io/netty/buffer/api/tests/BufferOffsetsTest.java b/buffer/src/test/java/io/netty/buffer/api/tests/BufferOffsetsTest.java
@@ -178,4 +178,50 @@ void readableBytesMustMatchWhatWasWritten(Fixture fixture) {
             assertEquals(Long.BYTES - Short.BYTES, buf.readableBytes());
         }
     }
+
+    @ParameterizedTest
+    @MethodSource("allocators")
+    void skipReadable(Fixture fixture) {
+        skipReadable(fixture, 32, 16, 8, 8);
+    }
+
+    @ParameterizedTest
+    @MethodSource("allocators")
+    void skipReadableNegative(Fixture fixture) {
+        skipReadable(fixture, 16, 8, 4, -2);
+    }
+
+    private void skipReadable(Fixture fixture, int capacity, int writeBytes, int readBytes, int offset) {
+        try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(capacity)) {
+            writeRandomBytes(buf, writeBytes);
+
+            for (int i = 0; i < readBytes; i++) {
+                buf.readByte();
+            }
+
+            buf.skipReadable(offset);
+            assertEquals(readBytes + offset, buf.readerOffset());
+        }
+    }
+
+    @ParameterizedTest
+    @MethodSource("allocators")
+    void skipWritable(Fixture fixture) {
+        skipWritable(fixture, 32, 16, 8);
+    }
+
+    @ParameterizedTest
+    @MethodSource("allocators")
+    void skipWritableNegative(Fixture fixture) {
+        skipWritable(fixture, 16, 8, -2);
+    }
+
+    private void skipWritable(Fixture fixture, int capacity, int writeBytes, int offset) {
+        try (BufferAllocator allocator = fixture.createAllocator(); Buffer buf = allocator.allocate(capacity)) {
+            writeRandomBytes(buf, writeBytes);
+
+            buf.skipWritable(offset);
+            assertEquals(writeBytes + offset, buf.writerOffset());
+        }
+    }
 }