From abfb31af02951c456ac0c571011fc5b892855cc5 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Tue, 7 Jan 2025 13:21:59 -0800 Subject: [PATCH] Make `UnsignedBytes.lexicographicalComparator()` use `Arrays.compareUnsigned` when it's available. This provides another [alternative to using `Unsafe`](https://github.com/google/guava/issues/6806). And port the benchmark to JMH, which for now means making it Google-internal. (Not that we have our _Caliper_ benchmarks actually _running_ externally, either, IIRC.) The benchmarks suggest that the old, `Unsafe`-based implementation is faster up to 64 elements or so but that it's a matter of only about a nanosecond. The new implementation pulls ahead for larger arrays, including an advantage of about 5-10 ns for 1024 elements. For now, I've included this implementation only in the JRE flavor of Guava. We could include it in the Android flavor, too, to see if it helps [under API Level 33+](https://developer.android.com/reference/java/util/Arrays#compareUnsigned(byte[],%20byte[])). But we really would want to do yet more benchmarking for that. RELNOTES=n/a PiperOrigin-RevId: 713020393 --- .../primitives/UnsignedBytesBenchmark.java | 107 ------------------ .../common/primitives/UnsignedBytesTest.java | 17 ++- .../common/primitives/UnsignedBytes.java | 46 ++++++++ 3 files changed, 58 insertions(+), 112 deletions(-) delete mode 100644 guava-tests/benchmark/com/google/common/primitives/UnsignedBytesBenchmark.java diff --git a/guava-tests/benchmark/com/google/common/primitives/UnsignedBytesBenchmark.java b/guava-tests/benchmark/com/google/common/primitives/UnsignedBytesBenchmark.java deleted file mode 100644 index 3b4fda5a7f28..000000000000 --- a/guava-tests/benchmark/com/google/common/primitives/UnsignedBytesBenchmark.java +++ /dev/null @@ -1,107 +0,0 @@ -/* - * Copyright (C) 2010 The Guava Authors - * - * Licensed 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 com.google.common.primitives; - -import com.google.caliper.BeforeExperiment; -import com.google.caliper.Benchmark; -import com.google.caliper.Param; -import java.util.Arrays; -import java.util.Comparator; -import java.util.Random; -import org.jspecify.annotations.NullUnmarked; - -/** - * Microbenchmark for {@link UnsignedBytes}. - * - * @author Hiroshi Yamauchi - */ -@NullUnmarked -public class UnsignedBytesBenchmark { - - private byte[] ba1; - private byte[] ba2; - private byte[] ba3; - private byte[] ba4; - private Comparator javaImpl; - private Comparator unsafeImpl; - - // 4, 8, 64, 1K, 1M, 1M (unaligned), 64M, 64M (unaligned) - // @Param({"4", "8", "64", "1024", "1048576", "1048577", "6710884", "6710883"}) - @Param({"4", "8", "64", "1024"}) - private int length; - - @BeforeExperiment - void setUp() throws Exception { - Random r = new Random(); - ba1 = new byte[length]; - r.nextBytes(ba1); - ba2 = Arrays.copyOf(ba1, ba1.length); - // Differ at the last element - ba3 = Arrays.copyOf(ba1, ba1.length); - ba4 = Arrays.copyOf(ba1, ba1.length); - ba3[ba1.length - 1] = (byte) 43; - ba4[ba1.length - 1] = (byte) 42; - - javaImpl = UnsignedBytes.lexicographicalComparatorJavaImpl(); - unsafeImpl = UnsignedBytes.LexicographicalComparatorHolder.UnsafeComparator.INSTANCE; - } - - @Benchmark - void longEqualJava(int reps) { - for (int i = 0; i < reps; ++i) { - if (javaImpl.compare(ba1, ba2) != 0) { - throw new Error(); // deoptimization - } - } - } - - @Benchmark - void longEqualUnsafe(int reps) { - for (int i = 0; i < reps; ++i) { - if (unsafeImpl.compare(ba1, ba2) != 0) { - throw new Error(); // deoptimization - } - } - } - - @Benchmark - void diffLastJava(int reps) { - for (int i = 0; i < reps; ++i) { - if (javaImpl.compare(ba3, ba4) == 0) { - throw new Error(); // deoptimization - } - } - } - - @Benchmark - void diffLastUnsafe(int reps) { - for (int i = 0; i < reps; ++i) { - if (unsafeImpl.compare(ba3, ba4) == 0) { - throw new Error(); // deoptimization - } - } - } - - /* - try { - UnsignedBytesBenchmark bench = new UnsignedBytesBenchmark(); - bench.length = 1024; - bench.setUp(); - bench.timeUnsafe(100000); - } catch (Exception e) { - }*/ -} diff --git a/guava-tests/test/com/google/common/primitives/UnsignedBytesTest.java b/guava-tests/test/com/google/common/primitives/UnsignedBytesTest.java index da00a4274088..85f8e98439b4 100644 --- a/guava-tests/test/com/google/common/primitives/UnsignedBytesTest.java +++ b/guava-tests/test/com/google/common/primitives/UnsignedBytesTest.java @@ -16,6 +16,7 @@ package com.google.common.primitives; +import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION; import static com.google.common.primitives.UnsignedBytes.max; import static com.google.common.primitives.UnsignedBytes.min; import static com.google.common.truth.Truth.assertThat; @@ -242,12 +243,14 @@ public void testLexicographicalComparatorChoice() throws Exception { Comparator defaultComparator = UnsignedBytes.lexicographicalComparator(); assertThat(defaultComparator).isNotNull(); assertThat(UnsignedBytes.lexicographicalComparator()).isSameInstanceAs(defaultComparator); - if (unsafeComparatorAvailable()) { - assertThat(Class.forName(unsafeComparatorClassName())) - .isSameInstanceAs(defaultComparator.getClass()); + if (!isJava8()) { + assertThat(defaultComparator.getClass()) + .isEqualTo(UnsignedBytes.ArraysCompareUnsignedComparator.class); + } else if (unsafeComparatorAvailable()) { + assertThat(defaultComparator.getClass()) + .isEqualTo(Class.forName(unsafeComparatorClassName())); } else { - assertThat(UnsignedBytes.lexicographicalComparatorJavaImpl()) - .isSameInstanceAs(defaultComparator); + assertThat(defaultComparator).isEqualTo(UnsignedBytes.lexicographicalComparatorJavaImpl()); } } @@ -360,4 +363,8 @@ public void testSortDescendingIndexed() { public void testNulls() { new NullPointerTester().testAllPublicStaticMethods(UnsignedBytes.class); } + + private static boolean isJava8() { + return JAVA_SPECIFICATION_VERSION.value().equals("1.8"); + } } diff --git a/guava/src/com/google/common/primitives/UnsignedBytes.java b/guava/src/com/google/common/primitives/UnsignedBytes.java index 377c2d61bebb..56956de85095 100644 --- a/guava/src/com/google/common/primitives/UnsignedBytes.java +++ b/guava/src/com/google/common/primitives/UnsignedBytes.java @@ -23,6 +23,7 @@ import com.google.common.annotations.J2ktIncompatible; import com.google.common.annotations.VisibleForTesting; import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.j2objc.annotations.J2ObjCIncompatible; import java.lang.reflect.Field; import java.nio.ByteOrder; import java.security.AccessController; @@ -30,6 +31,7 @@ import java.security.PrivilegedExceptionAction; import java.util.Arrays; import java.util.Comparator; +import org.jspecify.annotations.Nullable; import sun.misc.Unsafe; /** @@ -439,6 +441,12 @@ public String toString() { * to do so. */ static Comparator getBestComparator() { + Comparator arraysCompareUnsignedComparator = + ArraysCompareUnsignedComparatorMaker.INSTANCE.tryMakeArraysCompareUnsignedComparator(); + if (arraysCompareUnsignedComparator != null) { + return arraysCompareUnsignedComparator; + } + try { Class theClass = Class.forName(UNSAFE_COMPARATOR_NAME); @@ -455,6 +463,44 @@ static Comparator getBestComparator() { } } + private enum ArraysCompareUnsignedComparatorMaker { + INSTANCE { + /** Implementation used by non-J2ObjC environments. */ + // We use Arrays.compareUnsigned only after confirming that it's available at runtime. + @SuppressWarnings("Java8ApiChecker") + @IgnoreJRERequirement + @Override + @J2ObjCIncompatible + @Nullable Comparator tryMakeArraysCompareUnsignedComparator() { + try { + // Compare AbstractFuture.VarHandleAtomicHelperMaker. + Arrays.class.getMethod("compareUnsigned", byte[].class, byte[].class); + } catch (NoSuchMethodException beforeJava9) { + return null; + } + return ArraysCompareUnsignedComparator.INSTANCE; + } + }; + + /** Implementation used by J2ObjC environments, overridden for other environments. */ + @Nullable Comparator tryMakeArraysCompareUnsignedComparator() { + return null; + } + } + + @J2ObjCIncompatible + enum ArraysCompareUnsignedComparator implements Comparator { + INSTANCE; + + @Override + // We use the class only after confirming that Arrays.compareUnsigned is available at runtime. + @SuppressWarnings("Java8ApiChecker") + @IgnoreJRERequirement + public int compare(byte[] left, byte[] right) { + return Arrays.compareUnsigned(left, right); + } + } + private static byte flip(byte b) { return (byte) (b ^ 0x80); }