You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "charlesconnell (via GitHub)" <gi...@apache.org> on 2025/10/27 19:33:27 UTC
[PR] HBASE-29688: Remove many uses of Unsafe [hbase]
charlesconnell opened a new pull request, #7418:
URL: https://github.com/apache/hbase/pull/7418
Java offers `VarHandle` to facilitate fast access to memory, in place of the `Unsafe#get*` and `Unsafe#put*` methods. It can be used as almost a drop-in replacement, and the performance is similar. `VarHandle` was introduced in Java 9. HBase 2.x supports Java 8, so we cannot use `VarHandle` in 2.x releases, but we can prepare to use it in 3.x releases, which target Java 17. This will eliminate most uses of `Unsafe` in HBase. Most remaining uses of `Unsafe` are for copying memory regions. There is an alternative available for that, `MemorySegment`, that is available in Java 22 and higher, but we'll tackle that much later.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466852703
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/ByteBuffAllocator.java:
##########
@@ -349,12 +348,7 @@ public ByteBuff allocate(int size) {
* Free all direct buffers if allocated, mainly used for testing.
*/
public void clean() {
- while (!buffers.isEmpty()) {
- ByteBuffer b = buffers.poll();
- if (b.isDirect()) {
- UnsafeAccess.freeDirectBuffer(b);
- }
- }
Review Comment:
This was only used in testing, and I think it'll be okay to just let the GC free the buffers
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466865060
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -58,28 +73,6 @@ static abstract class Comparer {
abstract int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2);
}
- static abstract class Converter {
Review Comment:
I don't think we'll do better than VarHandle at byte/primitive conversion, so multiple Converter implementations are not needed. I'm simplifying ByteBufferUtils by removing the Converter interface, and putting the implementations directly in the ByteBufferUtils methods where needed.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466881165
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/Bytes.java:
##########
@@ -124,7 +126,18 @@ public class Bytes implements Comparable<Bytes> {
// SizeOf which uses java.lang.instrument says 24 bytes. (3 longs?)
public static final int ESTIMATED_HEAP_TAX = 16;
- static final boolean UNSAFE_UNALIGNED = HBasePlatformDependent.unaligned();
+ /*
Review Comment:
Many of my changes in `ByteBufferUtils` have counterpart changes in `Bytes`. These are all the same, except they operate on byte arrays instead of ByteBuffers.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466882255
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/UnsafeAccess.java:
##########
@@ -50,249 +46,6 @@ public final class UnsafeAccess {
private UnsafeAccess() {
}
- // APIs to read primitive data from a byte[] using Unsafe way
Review Comment:
Most of the methods in UnsafeAccess are no longer used, and I'm deleting them
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466885545
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestBytesWoUnsafe.java:
##########
@@ -1,41 +0,0 @@
-/*
- * 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.hadoop.hbase.util;
-
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.mockito.Mockito.mockStatic;
-
-import org.apache.hadoop.hbase.testclassification.MiscTests;
-import org.apache.hadoop.hbase.testclassification.SmallTests;
-import org.apache.hadoop.hbase.unsafe.HBasePlatformDependent;
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Tag;
-import org.mockito.MockedStatic;
-
-@Tag(MiscTests.TAG)
-@Tag(SmallTests.TAG)
-public class TestBytesWoUnsafe extends BytesTestBase {
Review Comment:
`Bytes` no longer uses `Unsafe` at all, so this test doesn't make sense
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466872773
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -88,349 +81,150 @@ abstract int findCommonPrefix(ByteBuffer left, int leftOffset, int leftLength, B
int rightOffset, int rightLength);
}
+ /**
+ * <a href=
+ * "https://github.com/google/guava/blob/v21.0/guava/src/com/google/common/primitives/UnsignedBytes.java#L362">Adapted
+ * from Guava</a>
+ */
static class ComparerHolder {
- static final String UNSAFE_COMPARER_NAME = ComparerHolder.class.getName() + "$UnsafeComparer";
-
static final Comparer BEST_COMPARER = getBestComparer();
static Comparer getBestComparer() {
- try {
- Class<? extends Comparer> theClass =
- Class.forName(UNSAFE_COMPARER_NAME).asSubclass(Comparer.class);
-
- return theClass.getConstructor().newInstance();
- } catch (Throwable t) { // ensure we really catch *everything*
- return PureJavaComparer.INSTANCE;
- }
+ return VarHandleComparer.INSTANCE;
}
- static final class PureJavaComparer extends Comparer {
- static final PureJavaComparer INSTANCE = new PureJavaComparer();
+ static final class VarHandleComparer extends Comparer {
- private PureJavaComparer() {
- }
+ static final VarHandleComparer INSTANCE = new VarHandleComparer();
+ private static final int STRIDE = Long.BYTES;
@Override
- public int compareTo(byte[] buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- int end1 = o1 + l1;
- int end2 = o2 + l2;
- for (int i = o1, j = o2; i < end1 && j < end2; i++, j++) {
- int a = buf1[i] & 0xFF;
- int b = buf2.get(j) & 0xFF;
- if (a != b) {
- return a - b;
+ int compareTo(byte[] buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
+ final int minLength = Math.min(l1, l2);
+ int strideLimit = minLength & ~(STRIDE - 1);
+ int i;
+
+ for (i = 0; i < strideLimit; i += STRIDE) {
+ // big-endian fetches are faster than little-endian because the bytes don't need to get
+ // reversed
+ long lw = (long) BYTE_ARRAY_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf1, o1 + i);
+ long rw = (long) BYTE_BUFFER_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf2, o2 + i);
+ if (lw != rw) {
+ return ((lw + Long.MIN_VALUE) < (rw + Long.MIN_VALUE)) ? -1 : 1;
}
}
- return l1 - l2;
- }
- @Override
- public int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- int end1 = o1 + l1;
- int end2 = o2 + l2;
- for (int i = o1, j = o2; i < end1 && j < end2; i++, j++) {
- int a = buf1.get(i) & 0xFF;
- int b = buf2.get(j) & 0xFF;
- if (a != b) {
- return a - b;
+ // The epilogue to cover the last (minLength % stride) elements.
+ for (; i < minLength; i++) {
+ int il = buf1[o1 + i] & 0xFF;
+ int ir = buf2.get(o2 + i) & 0xFF;
+ if (il != ir) {
+ return il - ir;
}
}
return l1 - l2;
}
- }
-
- static final class UnsafeComparer extends Comparer {
-
- public UnsafeComparer() {
- }
-
- static {
- if (!UNSAFE_UNALIGNED) {
- throw new Error();
- }
- }
-
- @Override
- public int compareTo(byte[] buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- long offset2Adj;
- Object refObj2 = null;
- if (buf2.isDirect()) {
- offset2Adj = o2 + UnsafeAccess.directBufferAddress(buf2);
- } else {
- offset2Adj = o2 + buf2.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
- refObj2 = buf2.array();
- }
- return compareToUnsafe(buf1, o1 + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET, l1, refObj2,
- offset2Adj, l2);
- }
@Override
public int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- long offset1Adj, offset2Adj;
- Object refObj1 = null, refObj2 = null;
- if (buf1.isDirect()) {
- offset1Adj = o1 + UnsafeAccess.directBufferAddress(buf1);
- } else {
- offset1Adj = o1 + buf1.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
- refObj1 = buf1.array();
- }
- if (buf2.isDirect()) {
- offset2Adj = o2 + UnsafeAccess.directBufferAddress(buf2);
- } else {
- offset2Adj = o2 + buf2.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
- refObj2 = buf2.array();
+ final int minLength = Math.min(l1, l2);
+ int strideLimit = minLength & ~(STRIDE - 1);
+ int i;
+
+ for (i = 0; i < strideLimit; i += STRIDE) {
+ // big-endian fetches are faster than little-endian because the bytes don't need to get
+ // reversed
+ long lw = (long) BYTE_BUFFER_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf1, o1 + i);
+ long rw = (long) BYTE_BUFFER_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf2, o2 + i);
+ if (lw != rw) {
+ return ((lw + Long.MIN_VALUE) < (rw + Long.MIN_VALUE)) ? -1 : 1;
+ }
}
- return compareToUnsafe(refObj1, offset1Adj, l1, refObj2, offset2Adj, l2);
- }
- }
- }
-
- static class ConverterHolder {
- static final String UNSAFE_CONVERTER_NAME =
- ConverterHolder.class.getName() + "$UnsafeConverter";
- static final Converter BEST_CONVERTER = getBestConverter();
-
- static Converter getBestConverter() {
- try {
- Class<? extends Converter> theClass =
- Class.forName(UNSAFE_CONVERTER_NAME).asSubclass(Converter.class);
-
- // yes, UnsafeComparer does implement Comparer<byte[]>
- return theClass.getConstructor().newInstance();
- } catch (Throwable t) { // ensure we really catch *everything*
- return PureJavaConverter.INSTANCE;
- }
- }
-
- static final class PureJavaConverter extends Converter {
- static final PureJavaConverter INSTANCE = new PureJavaConverter();
-
- private PureJavaConverter() {
- }
-
- @Override
- short toShort(ByteBuffer buffer, int offset) {
- return buffer.getShort(offset);
- }
-
- @Override
- int toInt(ByteBuffer buffer) {
- return buffer.getInt();
- }
-
- @Override
- int toInt(ByteBuffer buffer, int offset) {
- return buffer.getInt(offset);
- }
-
- @Override
- long toLong(ByteBuffer buffer, int offset) {
- return buffer.getLong(offset);
- }
-
- @Override
- void putInt(ByteBuffer buffer, int val) {
- buffer.putInt(val);
- }
-
- @Override
- int putInt(ByteBuffer buffer, int index, int val) {
- buffer.putInt(index, val);
- return index + Bytes.SIZEOF_INT;
- }
- @Override
- void putShort(ByteBuffer buffer, short val) {
- buffer.putShort(val);
- }
-
- @Override
- int putShort(ByteBuffer buffer, int index, short val) {
- buffer.putShort(index, val);
- return index + Bytes.SIZEOF_SHORT;
- }
-
- @Override
- void putLong(ByteBuffer buffer, long val) {
- buffer.putLong(val);
- }
-
- @Override
- int putLong(ByteBuffer buffer, int index, long val) {
- buffer.putLong(index, val);
- return index + Bytes.SIZEOF_LONG;
- }
- }
-
- static final class UnsafeConverter extends Converter {
-
- public UnsafeConverter() {
- }
-
- static {
- if (!UNSAFE_UNALIGNED) {
- throw new Error();
+ // The epilogue to cover the last (minLength % stride) elements.
+ for (; i < minLength; i++) {
+ int il = buf1.get(o1 + i) & 0xFF;
+ int ir = buf2.get(o2 + i) & 0xFF;
+ if (il != ir) {
+ return il - ir;
+ }
}
- }
-
- @Override
- short toShort(ByteBuffer buffer, int offset) {
- return UnsafeAccess.toShort(buffer, offset);
- }
-
- @Override
- int toInt(ByteBuffer buffer) {
- int i = UnsafeAccess.toInt(buffer, buffer.position());
- buffer.position(buffer.position() + Bytes.SIZEOF_INT);
- return i;
- }
-
- @Override
- int toInt(ByteBuffer buffer, int offset) {
- return UnsafeAccess.toInt(buffer, offset);
- }
-
- @Override
- long toLong(ByteBuffer buffer, int offset) {
- return UnsafeAccess.toLong(buffer, offset);
- }
-
- @Override
- void putInt(ByteBuffer buffer, int val) {
- int newPos = UnsafeAccess.putInt(buffer, buffer.position(), val);
- buffer.position(newPos);
- }
-
- @Override
- int putInt(ByteBuffer buffer, int index, int val) {
- return UnsafeAccess.putInt(buffer, index, val);
- }
-
- @Override
- void putShort(ByteBuffer buffer, short val) {
- int newPos = UnsafeAccess.putShort(buffer, buffer.position(), val);
- buffer.position(newPos);
- }
-
- @Override
- int putShort(ByteBuffer buffer, int index, short val) {
- return UnsafeAccess.putShort(buffer, index, val);
- }
-
- @Override
- void putLong(ByteBuffer buffer, long val) {
- int newPos = UnsafeAccess.putLong(buffer, buffer.position(), val);
- buffer.position(newPos);
- }
-
- @Override
- int putLong(ByteBuffer buffer, int index, long val) {
- return UnsafeAccess.putLong(buffer, index, val);
+ return l1 - l2;
}
}
}
static class CommonPrefixerHolder {
- static final String UNSAFE_COMMON_PREFIXER_NAME =
- CommonPrefixerHolder.class.getName() + "$UnsafeCommonPrefixer";
-
static final CommonPrefixer BEST_COMMON_PREFIXER = getBestCommonPrefixer();
static CommonPrefixer getBestCommonPrefixer() {
Review Comment:
I am keeping the CommonPrefixer interface, because when the Vector API (hopefully) gets standardized, we can try to load a Vector-based CommonPrefixer at runtime, and maybe it will be even faster.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466883745
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/util/ByteBufferUtilsTestBase.java:
##########
@@ -505,6 +505,174 @@ public void testCompareTo() {
assertEquals(0, result);
}
+ @Test
Review Comment:
I'm adding many unit tests to exercise the functionality I'm reimplementing, but
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/util/ByteBufferUtilsTestBase.java:
##########
@@ -505,6 +505,174 @@ public void testCompareTo() {
assertEquals(0, result);
}
+ @Test
Review Comment:
I'm adding many unit tests to exercise the functionality I'm reimplementing
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "Apache9 (via GitHub)" <gi...@apache.org>.
Apache9 commented on PR #7418:
URL: https://github.com/apache/hbase/pull/7418#issuecomment-3454069450
I prefer we do this after 3.0.0 release, where the community will start to move up to 3.x and JDK17.
For now, we'd better align the code between 3.x and 2.x as much as possible to reduce the maintanence overhead.
Anyway, I'm +1 on removing the usage of unsafe as much as possible while we can retain the performance.
Thanks.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell closed pull request #7418: HBASE-29688: Remove many uses of Unsafe
URL: https://github.com/apache/hbase/pull/7418
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466865060
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -58,28 +73,6 @@ static abstract class Comparer {
abstract int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2);
}
- static abstract class Converter {
Review Comment:
I don't think we'll do better than VarHandle, mulitple Converting implementations are not needed. I'm simplifying ByteBufferUtils by removing the Converter interface, and putting the implementations directly in the ByteBufferUtils methods where needed.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -58,28 +73,6 @@ static abstract class Comparer {
abstract int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2);
}
- static abstract class Converter {
Review Comment:
I don't think we'll do better than VarHandle, mulitple Converter implementations are not needed. I'm simplifying ByteBufferUtils by removing the Converter interface, and putting the implementations directly in the ByteBufferUtils methods where needed.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466865060
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -58,28 +73,6 @@ static abstract class Comparer {
abstract int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2);
}
- static abstract class Converter {
Review Comment:
I don't think we'll do better than VarHandle, so multiple Converter implementations are not needed. I'm simplifying ByteBufferUtils by removing the Converter interface, and putting the implementations directly in the ByteBufferUtils methods where needed.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466889141
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -580,11 +374,9 @@ public static void putInt(OutputStream out, final int value) throws IOException
}
public static byte toByte(ByteBuffer buffer, int offset) {
- if (UNSAFE_AVAIL) {
- return UnsafeAccess.toByte(buffer, offset);
- } else {
- return buffer.get(offset);
- }
+ // For some reason there is no way to get one byte from a ByteBuffer via a VarHandle, so
+ // for now just do things the slow way.
+ return buffer.get(offset);
Review Comment:
This is one annoying place where VarHandle doesn't work
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on PR #7418:
URL: https://github.com/apache/hbase/pull/7418#issuecomment-3456276951
Understood, that makes sense. I'll bring this back up after 3.0.
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466872773
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -88,349 +81,150 @@ abstract int findCommonPrefix(ByteBuffer left, int leftOffset, int leftLength, B
int rightOffset, int rightLength);
}
+ /**
+ * <a href=
+ * "https://github.com/google/guava/blob/v21.0/guava/src/com/google/common/primitives/UnsignedBytes.java#L362">Adapted
+ * from Guava</a>
+ */
static class ComparerHolder {
- static final String UNSAFE_COMPARER_NAME = ComparerHolder.class.getName() + "$UnsafeComparer";
-
static final Comparer BEST_COMPARER = getBestComparer();
static Comparer getBestComparer() {
- try {
- Class<? extends Comparer> theClass =
- Class.forName(UNSAFE_COMPARER_NAME).asSubclass(Comparer.class);
-
- return theClass.getConstructor().newInstance();
- } catch (Throwable t) { // ensure we really catch *everything*
- return PureJavaComparer.INSTANCE;
- }
+ return VarHandleComparer.INSTANCE;
}
- static final class PureJavaComparer extends Comparer {
- static final PureJavaComparer INSTANCE = new PureJavaComparer();
+ static final class VarHandleComparer extends Comparer {
- private PureJavaComparer() {
- }
+ static final VarHandleComparer INSTANCE = new VarHandleComparer();
+ private static final int STRIDE = Long.BYTES;
@Override
- public int compareTo(byte[] buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- int end1 = o1 + l1;
- int end2 = o2 + l2;
- for (int i = o1, j = o2; i < end1 && j < end2; i++, j++) {
- int a = buf1[i] & 0xFF;
- int b = buf2.get(j) & 0xFF;
- if (a != b) {
- return a - b;
+ int compareTo(byte[] buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
+ final int minLength = Math.min(l1, l2);
+ int strideLimit = minLength & ~(STRIDE - 1);
+ int i;
+
+ for (i = 0; i < strideLimit; i += STRIDE) {
+ // big-endian fetches are faster than little-endian because the bytes don't need to get
+ // reversed
+ long lw = (long) BYTE_ARRAY_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf1, o1 + i);
+ long rw = (long) BYTE_BUFFER_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf2, o2 + i);
+ if (lw != rw) {
+ return ((lw + Long.MIN_VALUE) < (rw + Long.MIN_VALUE)) ? -1 : 1;
}
}
- return l1 - l2;
- }
- @Override
- public int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- int end1 = o1 + l1;
- int end2 = o2 + l2;
- for (int i = o1, j = o2; i < end1 && j < end2; i++, j++) {
- int a = buf1.get(i) & 0xFF;
- int b = buf2.get(j) & 0xFF;
- if (a != b) {
- return a - b;
+ // The epilogue to cover the last (minLength % stride) elements.
+ for (; i < minLength; i++) {
+ int il = buf1[o1 + i] & 0xFF;
+ int ir = buf2.get(o2 + i) & 0xFF;
+ if (il != ir) {
+ return il - ir;
}
}
return l1 - l2;
}
- }
-
- static final class UnsafeComparer extends Comparer {
-
- public UnsafeComparer() {
- }
-
- static {
- if (!UNSAFE_UNALIGNED) {
- throw new Error();
- }
- }
-
- @Override
- public int compareTo(byte[] buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- long offset2Adj;
- Object refObj2 = null;
- if (buf2.isDirect()) {
- offset2Adj = o2 + UnsafeAccess.directBufferAddress(buf2);
- } else {
- offset2Adj = o2 + buf2.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
- refObj2 = buf2.array();
- }
- return compareToUnsafe(buf1, o1 + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET, l1, refObj2,
- offset2Adj, l2);
- }
@Override
public int compareTo(ByteBuffer buf1, int o1, int l1, ByteBuffer buf2, int o2, int l2) {
- long offset1Adj, offset2Adj;
- Object refObj1 = null, refObj2 = null;
- if (buf1.isDirect()) {
- offset1Adj = o1 + UnsafeAccess.directBufferAddress(buf1);
- } else {
- offset1Adj = o1 + buf1.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
- refObj1 = buf1.array();
- }
- if (buf2.isDirect()) {
- offset2Adj = o2 + UnsafeAccess.directBufferAddress(buf2);
- } else {
- offset2Adj = o2 + buf2.arrayOffset() + UnsafeAccess.BYTE_ARRAY_BASE_OFFSET;
- refObj2 = buf2.array();
+ final int minLength = Math.min(l1, l2);
+ int strideLimit = minLength & ~(STRIDE - 1);
+ int i;
+
+ for (i = 0; i < strideLimit; i += STRIDE) {
+ // big-endian fetches are faster than little-endian because the bytes don't need to get
+ // reversed
+ long lw = (long) BYTE_BUFFER_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf1, o1 + i);
+ long rw = (long) BYTE_BUFFER_LONG_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buf2, o2 + i);
+ if (lw != rw) {
+ return ((lw + Long.MIN_VALUE) < (rw + Long.MIN_VALUE)) ? -1 : 1;
+ }
}
- return compareToUnsafe(refObj1, offset1Adj, l1, refObj2, offset2Adj, l2);
- }
- }
- }
-
- static class ConverterHolder {
- static final String UNSAFE_CONVERTER_NAME =
- ConverterHolder.class.getName() + "$UnsafeConverter";
- static final Converter BEST_CONVERTER = getBestConverter();
-
- static Converter getBestConverter() {
- try {
- Class<? extends Converter> theClass =
- Class.forName(UNSAFE_CONVERTER_NAME).asSubclass(Converter.class);
-
- // yes, UnsafeComparer does implement Comparer<byte[]>
- return theClass.getConstructor().newInstance();
- } catch (Throwable t) { // ensure we really catch *everything*
- return PureJavaConverter.INSTANCE;
- }
- }
-
- static final class PureJavaConverter extends Converter {
- static final PureJavaConverter INSTANCE = new PureJavaConverter();
-
- private PureJavaConverter() {
- }
-
- @Override
- short toShort(ByteBuffer buffer, int offset) {
- return buffer.getShort(offset);
- }
-
- @Override
- int toInt(ByteBuffer buffer) {
- return buffer.getInt();
- }
-
- @Override
- int toInt(ByteBuffer buffer, int offset) {
- return buffer.getInt(offset);
- }
-
- @Override
- long toLong(ByteBuffer buffer, int offset) {
- return buffer.getLong(offset);
- }
-
- @Override
- void putInt(ByteBuffer buffer, int val) {
- buffer.putInt(val);
- }
-
- @Override
- int putInt(ByteBuffer buffer, int index, int val) {
- buffer.putInt(index, val);
- return index + Bytes.SIZEOF_INT;
- }
- @Override
- void putShort(ByteBuffer buffer, short val) {
- buffer.putShort(val);
- }
-
- @Override
- int putShort(ByteBuffer buffer, int index, short val) {
- buffer.putShort(index, val);
- return index + Bytes.SIZEOF_SHORT;
- }
-
- @Override
- void putLong(ByteBuffer buffer, long val) {
- buffer.putLong(val);
- }
-
- @Override
- int putLong(ByteBuffer buffer, int index, long val) {
- buffer.putLong(index, val);
- return index + Bytes.SIZEOF_LONG;
- }
- }
-
- static final class UnsafeConverter extends Converter {
-
- public UnsafeConverter() {
- }
-
- static {
- if (!UNSAFE_UNALIGNED) {
- throw new Error();
+ // The epilogue to cover the last (minLength % stride) elements.
+ for (; i < minLength; i++) {
+ int il = buf1.get(o1 + i) & 0xFF;
+ int ir = buf2.get(o2 + i) & 0xFF;
+ if (il != ir) {
+ return il - ir;
+ }
}
- }
-
- @Override
- short toShort(ByteBuffer buffer, int offset) {
- return UnsafeAccess.toShort(buffer, offset);
- }
-
- @Override
- int toInt(ByteBuffer buffer) {
- int i = UnsafeAccess.toInt(buffer, buffer.position());
- buffer.position(buffer.position() + Bytes.SIZEOF_INT);
- return i;
- }
-
- @Override
- int toInt(ByteBuffer buffer, int offset) {
- return UnsafeAccess.toInt(buffer, offset);
- }
-
- @Override
- long toLong(ByteBuffer buffer, int offset) {
- return UnsafeAccess.toLong(buffer, offset);
- }
-
- @Override
- void putInt(ByteBuffer buffer, int val) {
- int newPos = UnsafeAccess.putInt(buffer, buffer.position(), val);
- buffer.position(newPos);
- }
-
- @Override
- int putInt(ByteBuffer buffer, int index, int val) {
- return UnsafeAccess.putInt(buffer, index, val);
- }
-
- @Override
- void putShort(ByteBuffer buffer, short val) {
- int newPos = UnsafeAccess.putShort(buffer, buffer.position(), val);
- buffer.position(newPos);
- }
-
- @Override
- int putShort(ByteBuffer buffer, int index, short val) {
- return UnsafeAccess.putShort(buffer, index, val);
- }
-
- @Override
- void putLong(ByteBuffer buffer, long val) {
- int newPos = UnsafeAccess.putLong(buffer, buffer.position(), val);
- buffer.position(newPos);
- }
-
- @Override
- int putLong(ByteBuffer buffer, int index, long val) {
- return UnsafeAccess.putLong(buffer, index, val);
+ return l1 - l2;
}
}
}
static class CommonPrefixerHolder {
- static final String UNSAFE_COMMON_PREFIXER_NAME =
- CommonPrefixerHolder.class.getName() + "$UnsafeCommonPrefixer";
-
static final CommonPrefixer BEST_COMMON_PREFIXER = getBestCommonPrefixer();
static CommonPrefixer getBestCommonPrefixer() {
Review Comment:
I am keeping the CommonPrefixer interface, because when the Vector API (hopefully) gets standardized, we can try to load a Vector-based CommonPrefixer at runtime
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466877652
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -1055,100 +847,23 @@ public static int compareTo(ByteBuffer buf1, int o1, int l1, byte[] buf2, int o2
return compareTo(buf2, o2, l2, buf1, o1, l1) * -1;
}
- static int compareToUnsafe(Object obj1, long o1, int l1, Object obj2, long o2, int l2) {
- final int stride = 8;
- final int minLength = Math.min(l1, l2);
- int strideLimit = minLength & ~(stride - 1);
- int i;
-
- /*
- * Compare 8 bytes at a time. Benchmarking shows comparing 8 bytes at a time is no slower than
- * comparing 4 bytes at a time even on 32-bit. On the other hand, it is substantially faster on
- * 64-bit.
- */
- for (i = 0; i < strideLimit; i += stride) {
- long lw = HBasePlatformDependent.getLong(obj1, o1 + (long) i);
- long rw = HBasePlatformDependent.getLong(obj2, o2 + (long) i);
- if (lw != rw) {
- if (!UnsafeAccess.LITTLE_ENDIAN) {
- return ((lw + Long.MIN_VALUE) < (rw + Long.MIN_VALUE)) ? -1 : 1;
- }
-
- /*
- * We want to compare only the first index where left[index] != right[index]. This
- * corresponds to the least significant nonzero byte in lw ^ rw, since lw and rw are
- * little-endian. Long.numberOfTrailingZeros(diff) tells us the least significant nonzero
- * bit, and zeroing out the first three bits of L.nTZ gives us the shift to get that least
- * significant nonzero byte. This comparison logic is based on UnsignedBytes from guava v21
- */
- int n = Long.numberOfTrailingZeros(lw ^ rw) & ~0x7;
- return ((int) ((lw >>> n) & 0xFF)) - ((int) ((rw >>> n) & 0xFF));
- }
- }
-
- // The epilogue to cover the last (minLength % stride) elements.
- for (; i < minLength; i++) {
- int il = (HBasePlatformDependent.getByte(obj1, o1 + i) & 0xFF);
- int ir = (HBasePlatformDependent.getByte(obj2, o2 + i) & 0xFF);
- if (il != ir) {
- return il - ir;
- }
- }
- return l1 - l2;
- }
-
- static int findCommonPrefixUnsafe(Object left, long leftOffset, int leftLength, Object right,
- long rightOffset, int rightLength) {
- final int stride = 8;
- final int minLength = Math.min(leftLength, rightLength);
- int strideLimit = minLength & ~(stride - 1);
- int result = 0;
- int i;
-
- for (i = 0; i < strideLimit; i += stride) {
- long lw = HBasePlatformDependent.getLong(left, leftOffset + (long) i);
- long rw = HBasePlatformDependent.getLong(right, rightOffset + (long) i);
-
- if (lw != rw) {
- if (!UnsafeAccess.LITTLE_ENDIAN) {
- return result + (Long.numberOfLeadingZeros(lw ^ rw) / Bytes.SIZEOF_LONG);
- } else {
- return result + (Long.numberOfTrailingZeros(lw ^ rw) / Bytes.SIZEOF_LONG);
- }
- } else {
- result += Bytes.SIZEOF_LONG;
- }
- }
-
- // The epilogue to cover the last (minLength % stride) elements.
- for (; i < minLength; i++) {
- byte il = HBasePlatformDependent.getByte(left, leftOffset + i);
- byte ir = HBasePlatformDependent.getByte(right, rightOffset + i);
- if (il != ir) {
- return result;
- } else {
- result++;
- }
- }
-
- return result;
- }
-
/**
* Reads a short value at the given buffer's offset.
* @param buffer input byte buffer to read
* @param offset input offset where short is
* @return short value at offset
*/
public static short toShort(ByteBuffer buffer, int offset) {
- return ConverterHolder.BEST_CONVERTER.toShort(buffer, offset);
+ return (short) BYTE_BUFFER_SHORT_VIEW_BIG_ENDIAN_VAR_HANDLE.get(buffer, offset);
Review Comment:
Here and in the similar methods below, I'm removing some indirection by calling the var handle directly
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29688: Remove many uses of Unsafe [hbase]
Posted by "charlesconnell (via GitHub)" <gi...@apache.org>.
charlesconnell commented on code in PR #7418:
URL: https://github.com/apache/hbase/pull/7418#discussion_r2466870965
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/ByteBufferUtils.java:
##########
@@ -88,349 +81,150 @@ abstract int findCommonPrefix(ByteBuffer left, int leftOffset, int leftLength, B
int rightOffset, int rightLength);
}
+ /**
+ * <a href=
+ * "https://github.com/google/guava/blob/v21.0/guava/src/com/google/common/primitives/UnsignedBytes.java#L362">Adapted
+ * from Guava</a>
+ */
static class ComparerHolder {
- static final String UNSAFE_COMPARER_NAME = ComparerHolder.class.getName() + "$UnsafeComparer";
-
static final Comparer BEST_COMPARER = getBestComparer();
static Comparer getBestComparer() {
Review Comment:
I am keeping the Comparer interface, because when the Vector API (hopefully) gets standardized, we can try to load a Vector-based comparer at runtime
--
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org