ByteBuf for Key instead of String for codec-memcache

Motivation: The key can be ByteBuf to avoid converting between ByteBuf and String. See netty#3689. Modifications: Replace the type of key with ByteBuf. Result: The type of key becomes ByteBuf.
jin716 · Feb 8, 2016 · 36aa119 · 36aa119
1 parent f43dc7d
commit 36aa119
Show file tree

Hide file tree

Showing 15 changed files with 84 additions and 70 deletions.
diff --git a/...e/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheDecoder.java b/...e/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheDecoder.java
@@ -24,12 +24,9 @@
 import io.netty.handler.codec.memcache.DefaultMemcacheContent;
 import io.netty.handler.codec.memcache.LastMemcacheContent;
 import io.netty.handler.codec.memcache.MemcacheContent;
-import io.netty.util.CharsetUtil;
 
 import java.util.List;
 
-import static io.netty.buffer.ByteBufUtil.*;
-
 /**
  * Decoder for both {@link BinaryMemcacheRequest} and {@link BinaryMemcacheResponse}.
  * <p/>
@@ -90,7 +87,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
                         return;
                     }
 
-                    currentMessage.setExtras(readBytes(ctx.alloc(), in, extrasLength));
+                    currentMessage.setExtras(in.readSlice(extrasLength).retain());
                 }
 
                 state = State.READ_KEY;
@@ -106,8 +103,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
                         return;
                     }
 
-                    currentMessage.setKey(in.toString(in.readerIndex(), keyLength, CharsetUtil.UTF_8));
-                    in.skipBytes(keyLength);
+                    currentMessage.setKey(in.readSlice(keyLength).retain());
                 }
                 out.add(currentMessage.retain());
                 state = State.READ_CONTENT;
@@ -135,7 +131,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
                         toRead = remainingLength;
                     }
 
-                    ByteBuf chunkBuffer = readBytes(ctx.alloc(), in, toRead);
+                    ByteBuf chunkBuffer = in.readSlice(toRead).retain();
 
                     MemcacheContent chunk;
                     if ((alreadyReadChunkSize += toRead) >= valueLength) {

diff --git a/...e/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheEncoder.java b/...e/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheEncoder.java
@@ -64,12 +64,12 @@ private static void encodeExtras(ByteBuf buf, ByteBuf extras) {
      * @param buf the {@link ByteBuf} to write into.
      * @param key the key to encode.
      */
-    private static void encodeKey(ByteBuf buf, String key) {
-        if (key == null || key.isEmpty()) {
+    private static void encodeKey(ByteBuf buf, ByteBuf key) {
+        if (key == null || !key.isReadable()) {
             return;
         }
 
-        buf.writeBytes(key.getBytes(CharsetUtil.UTF_8));
+        buf.writeBytes(key);
     }
 
     /**

diff --git a/...e/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java b/...e/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java
@@ -28,7 +28,7 @@ public abstract class AbstractBinaryMemcacheMessage
     /**
      * Contains the optional key.
      */
-    private String key;
+    private ByteBuf key;
 
     /**
      * Contains the optional extras.
@@ -50,13 +50,13 @@ public abstract class AbstractBinaryMemcacheMessage
      * @param key    the message key.
      * @param extras the message extras.
      */
-    protected AbstractBinaryMemcacheMessage(String key, ByteBuf extras) {
+    protected AbstractBinaryMemcacheMessage(ByteBuf key, ByteBuf extras) {
         this.key = key;
         this.extras = extras;
     }
 
     @Override
-    public String key() {
+    public ByteBuf key() {
         return key;
     }
 
@@ -66,13 +66,19 @@ public ByteBuf extras() {
     }
 
     @Override
-    public BinaryMemcacheMessage setKey(String key) {
+    public BinaryMemcacheMessage setKey(ByteBuf key) {
+        if (this.key != null) {
+            this.key.release();
+        }
         this.key = key;
         return this;
     }
 
     @Override
     public BinaryMemcacheMessage setExtras(ByteBuf extras) {
+        if (this.extras != null) {
+            this.extras.release();
+        }
         this.extras = extras;
         return this;
     }
@@ -179,6 +185,9 @@ public BinaryMemcacheMessage retain(int increment) {
 
     @Override
     protected void deallocate() {
+        if (key != null) {
+            key.release();
+        }
         if (extras != null) {
             extras.release();
         }
@@ -192,6 +201,9 @@ public BinaryMemcacheMessage touch() {
 
     @Override
     public BinaryMemcacheMessage touch(Object hint) {
+        if (key != null) {
+            key.touch(hint);
+        }
         if (extras != null) {
             extras.touch(hint);
         }

diff --git a/...-memcache/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheMessage.java b/...-memcache/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheMessage.java
@@ -160,14 +160,16 @@ public interface BinaryMemcacheMessage extends MemcacheMessage {
      *
      * @return the key of the document.
      */
-    String key();
+    ByteBuf key();
 
     /**
-     * Sets the key of the document.
+     * Sets the key of the document. {@link ByteBuf#release()} ownership of {@code key}
+     * is transferred to this {@link BinaryMemcacheMessage}.
      *
-     * @param key the key of the message.
+     * @param key the key of the message. {@link ByteBuf#release()} ownership is transferred
+     *            to this {@link BinaryMemcacheMessage}.
      */
-    BinaryMemcacheMessage setKey(String key);
+    BinaryMemcacheMessage setKey(ByteBuf key);
 
     /**
      * Returns a {@link ByteBuf} representation of the optional extras.
@@ -177,9 +179,11 @@ public interface BinaryMemcacheMessage extends MemcacheMessage {
     ByteBuf extras();
 
     /**
-     * Sets the extras buffer on the message.
+     * Sets the extras buffer on the message. {@link ByteBuf#release()} ownership of {@code extras}
+     * is transferred to this {@link BinaryMemcacheMessage}.
      *
-     * @param extras the extras buffer of the document.
+     * @param extras the extras buffer of the document. {@link ByteBuf#release()} ownership is transferred
+     *               to this {@link BinaryMemcacheMessage}.
      */
     BinaryMemcacheMessage setExtras(ByteBuf extras);
 

diff --git a/.../src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregator.java b/.../src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregator.java
@@ -53,9 +53,10 @@ protected FullMemcacheMessage beginAggregation(BinaryMemcacheMessage start, Byte
     }
 
     private static FullBinaryMemcacheRequest toFullRequest(BinaryMemcacheRequest request, ByteBuf content) {
+        ByteBuf key = request.key() == null ? null : request.key().retain();
         ByteBuf extras = request.extras() == null ? null : request.extras().retain();
         FullBinaryMemcacheRequest fullRequest =
-                new DefaultFullBinaryMemcacheRequest(request.key(), extras, content);
+                new DefaultFullBinaryMemcacheRequest(key, extras, content);
 
         fullRequest.setMagic(request.magic());
         fullRequest.setOpcode(request.opcode());
@@ -71,9 +72,10 @@ private static FullBinaryMemcacheRequest toFullRequest(BinaryMemcacheRequest req
     }
 
     private static FullBinaryMemcacheResponse toFullResponse(BinaryMemcacheResponse response, ByteBuf content) {
+        ByteBuf key = response.key() == null ? null : response.key().retain();
         ByteBuf extras = response.extras() == null ? null : response.extras().retain();
         FullBinaryMemcacheResponse fullResponse =
-                new DefaultFullBinaryMemcacheResponse(response.key(), extras, content);
+                new DefaultFullBinaryMemcacheResponse(key, extras, content);
 
         fullResponse.setMagic(response.magic());
         fullResponse.setOpcode(response.opcode());

diff --git a/...he/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheRequestDecoder.java b/...he/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheRequestDecoder.java
@@ -49,6 +49,6 @@ protected BinaryMemcacheRequest decodeHeader(ByteBuf in) {
 
     @Override
     protected BinaryMemcacheRequest buildInvalidMessage() {
-        return new DefaultBinaryMemcacheRequest("", Unpooled.EMPTY_BUFFER);
+        return new DefaultBinaryMemcacheRequest(Unpooled.EMPTY_BUFFER, Unpooled.EMPTY_BUFFER);
     }
 }
diff --git a/...e/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheResponseDecoder.java b/...e/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheResponseDecoder.java
@@ -49,6 +49,6 @@ protected BinaryMemcacheResponse decodeHeader(ByteBuf in) {
 
     @Override
     protected BinaryMemcacheResponse buildInvalidMessage() {
-        return new DefaultBinaryMemcacheResponse("", Unpooled.EMPTY_BUFFER);
+        return new DefaultBinaryMemcacheResponse(Unpooled.EMPTY_BUFFER, Unpooled.EMPTY_BUFFER);
     }
 }
diff --git a/...he/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java b/...he/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java
@@ -41,26 +41,17 @@ public DefaultBinaryMemcacheRequest() {
      *
      * @param key    the key to use.
      */
-    public DefaultBinaryMemcacheRequest(String key) {
+    public DefaultBinaryMemcacheRequest(ByteBuf key) {
         this(key, null);
     }
 
-    /**
-     * Create a new {@link DefaultBinaryMemcacheRequest} with the header and extras.
-     *
-     * @param extras the extras to use.
-     */
-    public DefaultBinaryMemcacheRequest(ByteBuf extras) {
-        this(null, extras);
-    }
-
     /**
      * Create a new {@link DefaultBinaryMemcacheRequest} with the header only.
      *
      * @param key    the key to use.
      * @param extras the extras to use.
      */
-    public DefaultBinaryMemcacheRequest(String key, ByteBuf extras) {
+    public DefaultBinaryMemcacheRequest(ByteBuf key, ByteBuf extras) {
         super(key, extras);
         setMagic(REQUEST_MAGIC_BYTE);
     }

diff --git a/...e/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java b/...e/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java
@@ -41,26 +41,17 @@ public DefaultBinaryMemcacheResponse() {
      *
      * @param key    the key to use
      */
-    public DefaultBinaryMemcacheResponse(String key) {
+    public DefaultBinaryMemcacheResponse(ByteBuf key) {
         this(key, null);
     }
 
-    /**
-     * Create a new {@link DefaultBinaryMemcacheResponse} with the header and extras.
-     *
-     * @param extras the extras to use.
-     */
-    public DefaultBinaryMemcacheResponse(ByteBuf extras) {
-        this(null, extras);
-    }
-
     /**
      * Create a new {@link DefaultBinaryMemcacheResponse} with the header, key and extras.
      *
      * @param key    the key to use.
      * @param extras the extras to use.
      */
-    public DefaultBinaryMemcacheResponse(String key, ByteBuf extras) {
+    public DefaultBinaryMemcacheResponse(ByteBuf key, ByteBuf extras) {
         super(key, extras);
         setMagic(RESPONSE_MAGIC_BYTE);
     }

diff --git a/...rc/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequest.java b/...rc/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequest.java
@@ -32,7 +32,7 @@ public class DefaultFullBinaryMemcacheRequest extends DefaultBinaryMemcacheReque
      * @param key    the key to use.
      * @param extras the extras to use.
      */
-    public DefaultFullBinaryMemcacheRequest(String key, ByteBuf extras) {
+    public DefaultFullBinaryMemcacheRequest(ByteBuf key, ByteBuf extras) {
         this(key, extras, Unpooled.buffer(0));
     }
 
@@ -43,7 +43,7 @@ public DefaultFullBinaryMemcacheRequest(String key, ByteBuf extras) {
      * @param extras  the extras to use.
      * @param content the content of the full request.
      */
-    public DefaultFullBinaryMemcacheRequest(String key, ByteBuf extras,
+    public DefaultFullBinaryMemcacheRequest(ByteBuf key, ByteBuf extras,
                                             ByteBuf content) {
         super(key, extras);
         if (content == null) {
@@ -91,19 +91,27 @@ protected void deallocate() {
 
     @Override
     public FullBinaryMemcacheRequest copy() {
+        ByteBuf key = key();
+        if (key != null) {
+            key = key.copy();
+        }
         ByteBuf extras = extras();
         if (extras != null) {
             extras = extras.copy();
         }
-        return new DefaultFullBinaryMemcacheRequest(key(), extras, content().copy());
+        return new DefaultFullBinaryMemcacheRequest(key, extras, content().copy());
     }
 
     @Override
     public FullBinaryMemcacheRequest duplicate() {
+        ByteBuf key = key();
+        if (key != null) {
+            key = key.duplicate();
+        }
         ByteBuf extras = extras();
         if (extras != null) {
             extras = extras.duplicate();
         }
-        return new DefaultFullBinaryMemcacheRequest(key(), extras, content().duplicate());
+        return new DefaultFullBinaryMemcacheRequest(key, extras, content().duplicate());
     }
 }
diff --git a/...c/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponse.java b/...c/main/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponse.java
@@ -32,7 +32,7 @@ public class DefaultFullBinaryMemcacheResponse extends DefaultBinaryMemcacheResp
      * @param key    the key to use.
      * @param extras the extras to use.
      */
-    public DefaultFullBinaryMemcacheResponse(String key, ByteBuf extras) {
+    public DefaultFullBinaryMemcacheResponse(ByteBuf key, ByteBuf extras) {
         this(key, extras, Unpooled.buffer(0));
     }
 
@@ -43,7 +43,7 @@ public DefaultFullBinaryMemcacheResponse(String key, ByteBuf extras) {
      * @param extras  the extras to use.
      * @param content the content of the full request.
      */
-    public DefaultFullBinaryMemcacheResponse(String key, ByteBuf extras,
+    public DefaultFullBinaryMemcacheResponse(ByteBuf key, ByteBuf extras,
         ByteBuf content) {
         super(key, extras);
         if (content == null) {
@@ -91,19 +91,27 @@ protected void deallocate() {
 
     @Override
     public FullBinaryMemcacheResponse copy() {
+        ByteBuf key = key();
+        if (key != null) {
+            key = key.copy();
+        }
         ByteBuf extras = extras();
         if (extras != null) {
             extras = extras.copy();
         }
-        return new DefaultFullBinaryMemcacheResponse(key(), extras, content().copy());
+        return new DefaultFullBinaryMemcacheResponse(key, extras, content().copy());
     }
 
     @Override
     public FullBinaryMemcacheResponse duplicate() {
+        ByteBuf key = key();
+        if (key != null) {
+            key = key.duplicate();
+        }
         ByteBuf extras = extras();
         if (extras != null) {
             extras = extras.duplicate();
         }
-        return new DefaultFullBinaryMemcacheResponse(key(), extras, content().duplicate());
+        return new DefaultFullBinaryMemcacheResponse(key, extras, content().duplicate());
     }
 }
diff --git a/...cache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheDecoderTest.java b/...cache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheDecoderTest.java
@@ -256,10 +256,10 @@ public void shouldRetainCurrentMessageWhenSendingItOut() {
                 new BinaryMemcacheRequestEncoder(),
                 new BinaryMemcacheRequestDecoder());
 
-        String key = "Netty";
+        ByteBuf key = Unpooled.copiedBuffer("Netty", CharsetUtil.UTF_8);
         ByteBuf extras = Unpooled.copiedBuffer("extras", CharsetUtil.UTF_8);
         BinaryMemcacheRequest request = new DefaultBinaryMemcacheRequest(key, extras);
-        request.setKeyLength((short) key.length());
+        request.setKeyLength((short) key.readableBytes());
         request.setExtrasLength((byte) extras.readableBytes());
 
         assertTrue(channel.writeOutbound(request));

diff --git a/...cache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheEncoderTest.java b/...cache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheEncoderTest.java
@@ -85,7 +85,7 @@ public void shouldEncodeExtras() {
         ByteBuf extras = Unpooled.copiedBuffer(extrasContent, CharsetUtil.UTF_8);
         int extrasLength = extras.readableBytes();
 
-        BinaryMemcacheRequest request = new DefaultBinaryMemcacheRequest(extras);
+        BinaryMemcacheRequest request = new DefaultBinaryMemcacheRequest(Unpooled.EMPTY_BUFFER, extras);
         request.setExtrasLength((byte) extrasLength);
 
         boolean result = channel.writeOutbound(request);
@@ -100,8 +100,8 @@ public void shouldEncodeExtras() {
 
     @Test
     public void shouldEncodeKey() {
-        String key = "netty";
-        int keyLength = key.length();
+        ByteBuf key = Unpooled.copiedBuffer("netty", CharsetUtil.UTF_8);
+        int keyLength = key.readableBytes();
 
         BinaryMemcacheRequest request = new DefaultBinaryMemcacheRequest(key);
         request.setKeyLength((byte) keyLength);
@@ -112,7 +112,7 @@ public void shouldEncodeKey() {
         ByteBuf written = channel.readOutbound();
         assertThat(written.readableBytes(), is(DEFAULT_HEADER_SIZE + keyLength));
         written.readBytes(DEFAULT_HEADER_SIZE);
-        assertThat(written.readBytes(keyLength).toString(CharsetUtil.UTF_8), equalTo(key));
+        assertThat(written.readBytes(keyLength).toString(CharsetUtil.UTF_8), equalTo("netty"));
         written.release();
     }