dcapwell commented on code in PR #2122: URL: https://github.com/apache/cassandra/pull/2122#discussion_r1096199243
########## test/unit/org/apache/cassandra/transport/DuplicateHeapBufferTest.java: ########## @@ -0,0 +1,107 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.transport; + +import java.nio.ByteBuffer; +import java.nio.ReadOnlyBufferException; + +import org.junit.Assert; +import org.junit.Test; + +import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufAllocator; +import io.netty.buffer.PooledByteBufAllocator; +import org.apache.cassandra.utils.memory.MemoryUtil; + +public class DuplicateHeapBufferTest Review Comment: spoke already, we should remove as you moved away from heap buffer copying ########## src/java/org/apache/cassandra/utils/memory/MemoryUtil.java: ########## @@ -34,19 +34,20 @@ private static final Unsafe unsafe; private static final Class<?> DIRECT_BYTE_BUFFER_CLASS, RO_DIRECT_BYTE_BUFFER_CLASS; - private static final long DIRECT_BYTE_BUFFER_ADDRESS_OFFSET; Review Comment: can you revert all changes? as you migrated away from the heap buffer duplicate logic this logic is no longer needed to be changed ########## src/java/org/apache/cassandra/transport/CBUtil.java: ########## @@ -466,7 +475,7 @@ public static void writeValue(byte[] bytes, ByteBuf cb) cb.writeBytes(bytes); } - public static void writeValue(ByteBuffer bytes, ByteBuf cb) + public static void writeValu(ByteBuffer bytes, ByteBuf cb) Review Comment: please revert ########## src/java/org/apache/cassandra/transport/CBUtil.java: ########## @@ -69,6 +69,15 @@ protected CharsetDecoder initialValue() } }; + private final static FastThreadLocal<ByteBuffer> localBuffer = new FastThreadLocal<ByteBuffer>() Review Comment: would be best to call it `localDirectBuffer` ########## src/java/org/apache/cassandra/transport/CBUtil.java: ########## @@ -478,7 +487,37 @@ public static void writeValue(ByteBuffer bytes, ByteBuf cb) cb.writeInt(remaining); if (remaining > 0) - cb.writeBytes(bytes.duplicate()); + { + cb.writeBytes(MemoryUtil.duplicateHeapByteBuffer(bytes, getLocalBuffer())); + } + } + + public static void writeValue(ByteBuffer src, ByteBuf dest) Review Comment: should not copy/paste an existing method... You "should" add a new method that can write a `ByteBuffer` to a `ByteBuf` without allocating, then have the one line `cb.writeBytes(bytes.duplicate());` call that method instead. ########## src/java/org/apache/cassandra/transport/CBUtil.java: ########## @@ -478,7 +487,37 @@ public static void writeValue(ByteBuffer bytes, ByteBuf cb) cb.writeInt(remaining); if (remaining > 0) - cb.writeBytes(bytes.duplicate()); + { + cb.writeBytes(MemoryUtil.duplicateHeapByteBuffer(bytes, getLocalBuffer())); + } + } + + public static void writeValue(ByteBuffer src, ByteBuf dest) + { + if (src == null) + { + dest.writeInt(-1); + return; + } + + int length = src.remaining(); + dest.writeInt(length); + + if (src.hasArray()) + { + byte[] array = src.array(); Review Comment: can you add a comment explaining why we do this when direct duplicates the buffer? You found a JDK bug, we should expose that to anyone reading so they know that there may be dragons to replicate ########## src/java/org/apache/cassandra/transport/CBUtil.java: ########## @@ -614,4 +653,9 @@ private static byte[] readRawBytes(ByteBuf cb, int length) return bytes; } + public static ByteBuffer getLocalBuffer() Review Comment: also can you rename to `getLocalDirectBuffer`? so callers are more clear ########## src/java/org/apache/cassandra/transport/CBUtil.java: ########## @@ -614,4 +653,9 @@ private static byte[] readRawBytes(ByteBuf cb, int length) return bytes; } + public static ByteBuffer getLocalBuffer() Review Comment: I would leave private for now -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
