From 0e387fffc96f170f6cc04e22dc1a68e6226bcd34 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Thu, 12 Dec 2024 12:08:11 +0530 Subject: [PATCH 1/3] Cleanup unreachable Java 8 code flows --- .../indexing/common/task/HadoopTask.java | 16 ++- .../indexing/overlord/ForkingTaskRunner.java | 4 +- .../indexing/common/task/HadoopTaskTest.java | 10 +- .../java/util/common/ByteBufferUtils.java | 59 ++--------- .../druid/java/util/common/Cleaners.java | 23 +---- .../java/util/common/DefineClassUtils.java | 99 +------------------ .../java/org/apache/druid/utils/JvmUtils.java | 5 - .../org/apache/druid/utils/RuntimeInfo.java | 2 +- 8 files changed, 21 insertions(+), 197 deletions(-) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java index 201024540e16..ed0826e60570 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java @@ -173,15 +173,13 @@ public static ClassLoader buildClassLoader(final List hadoopDependencyCo localClassLoaderURLs.addAll(Arrays.asList(hadoopLoader.getURLs())); } - ClassLoader parent = null; - if (JvmUtils.isIsJava9Compatible()) { - try { - // See also https://docs.oracle.com/en/java/javase/11/migrate/index.html#JSMIG-GUID-A868D0B9-026F-4D46-B979-901834343F9E - parent = (ClassLoader) ClassLoader.class.getMethod("getPlatformClassLoader").invoke(null); - } - catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { - throw new RuntimeException(e); - } + ClassLoader parent; + try { + // See also https://docs.oracle.com/en/java/javase/11/migrate/index.html#JSMIG-GUID-A868D0B9-026F-4D46-B979-901834343F9E + parent = (ClassLoader) ClassLoader.class.getMethod("getPlatformClassLoader").invoke(null); + } + catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { + throw new RuntimeException(e); } final ClassLoader classLoader = new URLClassLoader( localClassLoaderURLs.toArray(new URL[0]), diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java index 7e4dd6c39c22..1a34c76e33fb 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java @@ -244,9 +244,7 @@ public TaskStatus call() command.add(config.getJavaCommand()); - if (JvmUtils.majorVersion() >= 11) { - command.addAll(STRONG_ENCAPSULATION_PROPERTIES); - } + command.addAll(STRONG_ENCAPSULATION_PROPERTIES); command.add("-cp"); command.add(taskClasspath); diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java index 130258e85d4c..741208dd0db4 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java @@ -30,7 +30,6 @@ import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.java.util.common.granularity.Granularity; import org.apache.druid.timeline.DataSegment; -import org.apache.druid.utils.JvmUtils; import org.easymock.EasyMock; import org.joda.time.Interval; import org.junit.Assert; @@ -131,13 +130,8 @@ public TaskStatus runTask(TaskToolbox toolbox) public static void assertClassLoaderIsSingular(ClassLoader classLoader) { - if (JvmUtils.isIsJava9Compatible()) { - // See also https://docs.oracle.com/en/java/javase/11/migrate/index.html#JSMIG-GUID-A868D0B9-026F-4D46-B979-901834343F9E - Assert.assertEquals("PlatformClassLoader", classLoader.getParent().getClass().getSimpleName()); - } else { - // This is a check against the current HadoopTask which creates a single URLClassLoader with null parent - Assert.assertNull(classLoader.getParent()); - } + // See also https://docs.oracle.com/en/java/javase/11/migrate/index.html#JSMIG-GUID-A868D0B9-026F-4D46-B979-901834343F9E + Assert.assertEquals("PlatformClassLoader", classLoader.getParent().getClass().getSimpleName()); Assert.assertFalse(classLoader.getClass().getSimpleName().equals("ApplicationClassLoader")); Assert.assertTrue(classLoader instanceof URLClassLoader); diff --git a/processing/src/main/java/org/apache/druid/java/util/common/ByteBufferUtils.java b/processing/src/main/java/org/apache/druid/java/util/common/ByteBufferUtils.java index fb67c3238347..696f0b629c9c 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/ByteBufferUtils.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/ByteBufferUtils.java @@ -20,17 +20,14 @@ package org.apache.druid.java.util.common; import org.apache.druid.collections.ResourceHolder; -import org.apache.druid.utils.JvmUtils; import javax.annotation.Nullable; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; -import java.lang.reflect.Method; import java.nio.ByteBuffer; import java.nio.MappedByteBuffer; import java.util.Comparator; -import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -88,11 +85,12 @@ private static MethodHandle lookupUnmapMethodHandle() { final MethodHandles.Lookup lookup = MethodHandles.lookup(); try { - if (JvmUtils.isIsJava9Compatible()) { - return unmapJava9(lookup); - } else { - return unmapJava7Or8(lookup); - } + MethodHandle unmapper = lookup.findVirtual( + UnsafeUtils.theUnsafeClass(), + "invokeCleaner", + MethodType.methodType(void.class, ByteBuffer.class) + ); + return unmapper.bindTo(UnsafeUtils.theUnsafe()); } catch (ReflectiveOperationException | RuntimeException e1) { throw new UnsupportedOperationException("Unmapping is not supported on this platform, because internal " + @@ -100,51 +98,6 @@ private static MethodHandle lookupUnmapMethodHandle() } } - /** - * NB: while Druid no longer support Java 7, this method would still work with that version as well. - */ - private static MethodHandle unmapJava7Or8(MethodHandles.Lookup lookup) throws ReflectiveOperationException - { - // "Compile" a MethodHandle that is roughly equivalent to the following lambda: - // - // (ByteBuffer buffer) -> { - // sun.misc.Cleaner cleaner = ((java.nio.DirectByteBuffer) byteBuffer).cleaner(); - // if (nonNull(cleaner)) - // cleaner.clean(); - // else - // noop(cleaner); // the noop is needed because MethodHandles#guardWithTest always needs both if and else - // } - // - Class directBufferClass = Class.forName("java.nio.DirectByteBuffer"); - Method m = directBufferClass.getMethod("cleaner"); - m.setAccessible(true); - MethodHandle directBufferCleanerMethod = lookup.unreflect(m); - Class cleanerClass = directBufferCleanerMethod.type().returnType(); - MethodHandle cleanMethod = lookup.findVirtual(cleanerClass, "clean", MethodType.methodType(void.class)); - MethodHandle nonNullTest = lookup.findStatic(Objects.class, "nonNull", - MethodType.methodType(boolean.class, Object.class) - ).asType(MethodType.methodType(boolean.class, cleanerClass)); - MethodHandle noop = MethodHandles.dropArguments(MethodHandles.constant( - Void.class, - null - ).asType(MethodType.methodType(void.class)), 0, cleanerClass); - MethodHandle unmapper = MethodHandles.filterReturnValue( - directBufferCleanerMethod, - MethodHandles.guardWithTest(nonNullTest, cleanMethod, noop) - ).asType(MethodType.methodType(void.class, ByteBuffer.class)); - return unmapper; - } - - private static MethodHandle unmapJava9(MethodHandles.Lookup lookup) throws ReflectiveOperationException - { - MethodHandle unmapper = lookup.findVirtual( - UnsafeUtils.theUnsafeClass(), - "invokeCleaner", - MethodType.methodType(void.class, ByteBuffer.class) - ); - return unmapper.bindTo(UnsafeUtils.theUnsafe()); - } - /** * Same as {@link ByteBuffer#allocateDirect(int)}, but returns a closeable {@link ResourceHolder} that * frees the buffer upon close. diff --git a/processing/src/main/java/org/apache/druid/java/util/common/Cleaners.java b/processing/src/main/java/org/apache/druid/java/util/common/Cleaners.java index 2de6a3e140db..2fa4ebdfafbb 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/Cleaners.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/Cleaners.java @@ -19,8 +19,6 @@ package org.apache.druid.java.util.common; -import org.apache.druid.utils.JvmUtils; - import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; @@ -62,11 +60,7 @@ private static Cleaner takeMeToTheCleaners() { final MethodHandles.Lookup lookup = MethodHandles.lookup(); try { - if (JvmUtils.isIsJava9Compatible()) { - return lookupCleanerJava9(lookup); - } else { - return lookupCleanerJava8(lookup); - } + return lookupCleaner(lookup); } catch (ReflectiveOperationException | RuntimeException e) { throw new UnsupportedOperationException("Cleaning is not support on this platform, because internal " + @@ -74,7 +68,7 @@ private static Cleaner takeMeToTheCleaners() } } - private static Cleaner lookupCleanerJava9(MethodHandles.Lookup lookup) throws ReflectiveOperationException + private static Cleaner lookupCleaner(MethodHandles.Lookup lookup) throws ReflectiveOperationException { Class cleaner = Class.forName("java.lang.ref.Cleaner"); Class cleanable = Class.forName("java.lang.ref.Cleaner$Cleanable"); @@ -100,19 +94,6 @@ private static Cleaner lookupCleanerJava9(MethodHandles.Lookup lookup) throws Re return new CleanerImpl(register, clean); } - private static Cleaner lookupCleanerJava8(MethodHandles.Lookup lookup) throws ReflectiveOperationException - { - Class cleaner = Class.forName("sun.misc.Cleaner"); - MethodHandle register = lookup.findStatic( - cleaner, - "create", - MethodType.methodType(cleaner, Object.class, Runnable.class) - ); - - MethodHandle clean = lookup.findVirtual(cleaner, "clean", MethodType.methodType(void.class)); - return new CleanerImpl(register, clean); - } - public static Cleanable register(Object object, Runnable runnable) { if (CLEANER == null) { diff --git a/processing/src/main/java/org/apache/druid/java/util/common/DefineClassUtils.java b/processing/src/main/java/org/apache/druid/java/util/common/DefineClassUtils.java index d79c517ae3d1..5e3bd6626f7d 100644 --- a/processing/src/main/java/org/apache/druid/java/util/common/DefineClassUtils.java +++ b/processing/src/main/java/org/apache/druid/java/util/common/DefineClassUtils.java @@ -19,12 +19,9 @@ package org.apache.druid.java.util.common; -import org.apache.druid.utils.JvmUtils; - import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; -import java.security.ProtectionDomain; /** * This utility class provides a thin runtime abstraction to pick between @@ -44,11 +41,7 @@ public class DefineClassUtils Exception exception = null; try { MethodHandles.Lookup lookup = MethodHandles.lookup(); - if (JvmUtils.isIsJava9Compatible()) { - defineClass = defineClassJava9(lookup); - } else { - defineClass = defineClassJava8(lookup); - } + defineClass = getMethodHandle(lookup); } catch (ReflectiveOperationException | RuntimeException e) { exception = e; @@ -70,9 +63,8 @@ public class DefineClassUtils * return targetClassLookup.defineClass(byteCode); * } */ - private static MethodHandle defineClassJava9(MethodHandles.Lookup lookup) throws ReflectiveOperationException + private static MethodHandle getMethodHandle(MethodHandles.Lookup lookup) throws ReflectiveOperationException { - // this is getting meta MethodHandle defineClass = lookup.unreflect(MethodHandles.Lookup.class.getMethod("defineClass", byte[].class)); MethodHandle privateLookupIn = lookup.findStatic( @@ -94,93 +86,6 @@ private static MethodHandle defineClassJava9(MethodHandles.Lookup lookup) throws return defineClass; } - /** - * "Compile" a MethodHandle that is equivalent to: - * - * Class defineClass(Class targetClass, byte[] byteCode, String className) { - * return Unsafe.defineClass( - * className, - * byteCode, - * 0, - * byteCode.length, - * targetClass.getClassLoader(), - * targetClass.getProtectionDomain() - * ); - * } - */ - private static MethodHandle defineClassJava8(MethodHandles.Lookup lookup) throws ReflectiveOperationException - { - MethodHandle defineClass = lookup.findVirtual( - UnsafeUtils.theUnsafeClass(), - "defineClass", - MethodType.methodType( - Class.class, - String.class, - byte[].class, - int.class, - int.class, - ClassLoader.class, - ProtectionDomain.class - ) - ).bindTo(UnsafeUtils.theUnsafe()); - - MethodHandle getProtectionDomain = lookup.unreflect(Class.class.getMethod("getProtectionDomain")); - MethodHandle getClassLoader = lookup.unreflect(Class.class.getMethod("getClassLoader")); - - // apply getProtectionDomain and getClassLoader to the targetClass, modifying the methodHandle as follows: - // defineClass = (String className, byte[] byteCode, int offset, int length, Class class1, Class class2) -> - // defineClass(className, byteCode, offset, length, class1.getClassLoader(), class2.getProtectionDomain()) - defineClass = MethodHandles.filterArguments(defineClass, 5, getProtectionDomain); - defineClass = MethodHandles.filterArguments(defineClass, 4, getClassLoader); - - // duplicate the last argument to apply the methods above to the same class, modifying the methodHandle as follows: - // defineClass = (String className, byte[] byteCode, int offset, int length, Class targetClass) -> - // defineClass(className, byteCode, offset, length, targetClass, targetClass) - defineClass = MethodHandles.permuteArguments( - defineClass, - MethodType.methodType(Class.class, String.class, byte[].class, int.class, int.class, Class.class), - 0, 1, 2, 3, 4, 4 - ); - - // set offset argument to 0, modifying the methodHandle as follows: - // defineClass = (String className, byte[] byteCode, int length, Class targetClass) -> - // defineClass(className, byteCode, 0, length, targetClass) - defineClass = MethodHandles.insertArguments(defineClass, 2, (int) 0); - - // JDK8 does not implement MethodHandles.arrayLength, so we have to roll our own - MethodHandle arrayLength = lookup.findStatic( - lookup.lookupClass(), - "getArrayLength", - MethodType.methodType(int.class, byte[].class) - ); - - // apply arrayLength to the length argument, modifying the methodHandle as follows: - // defineClass = (String className, byte[] byteCode1, byte[] byteCode2, Class targetClass) -> - // defineClass(className, byteCode1, byteCode2.length, targetClass) - defineClass = MethodHandles.filterArguments(defineClass, 2, arrayLength); - - // duplicate the byteCode argument and reorder to match JDK9 signature, modifying the methodHandle as follows: - // defineClass = (Class targetClass, byte[] byteCode, String className) -> - // defineClass(className, byteCode, byteCode, targetClass) - defineClass = MethodHandles.permuteArguments( - defineClass, - MethodType.methodType(Class.class, Class.class, byte[].class, String.class), - 2, 1, 1, 0 - ); - - return defineClass; - } - - /** - * This method is referenced in Java 8 using method handle, therefore it is not actually unused, and shouldn't be - * removed (till Java 8 is supported) - */ - @SuppressWarnings("unused") // method is referenced and used in defineClassJava8 - static int getArrayLength(byte[] bytes) - { - return bytes.length; - } - public static Class defineClass( Class targetClass, byte[] byteCode, diff --git a/processing/src/main/java/org/apache/druid/utils/JvmUtils.java b/processing/src/main/java/org/apache/druid/utils/JvmUtils.java index 493ac901420d..696a15aaee28 100644 --- a/processing/src/main/java/org/apache/druid/utils/JvmUtils.java +++ b/processing/src/main/java/org/apache/druid/utils/JvmUtils.java @@ -71,11 +71,6 @@ public static int majorVersion() return MAJOR_VERSION; } - public static boolean isIsJava9Compatible() - { - return MAJOR_VERSION >= 9; - } - public static RuntimeInfo getRuntimeInfo() { return RUNTIME_INFO; diff --git a/processing/src/main/java/org/apache/druid/utils/RuntimeInfo.java b/processing/src/main/java/org/apache/druid/utils/RuntimeInfo.java index 86571eff9456..115787cfd666 100644 --- a/processing/src/main/java/org/apache/druid/utils/RuntimeInfo.java +++ b/processing/src/main/java/org/apache/druid/utils/RuntimeInfo.java @@ -50,7 +50,7 @@ public long getFreeHeapSizeBytes() public long getDirectMemorySizeBytes() { try { - Class vmClass = Class.forName(JvmUtils.majorVersion() >= 9 ? "jdk.internal.misc.VM" : "sun.misc.VM"); + Class vmClass = Class.forName("jdk.internal.misc.VM"); Object maxDirectMemoryObj = vmClass.getMethod("maxDirectMemory").invoke(null); if (maxDirectMemoryObj == null || !(maxDirectMemoryObj instanceof Number)) { From 5c2145d9dcf5152af768e82d47651b3a34fd6753 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Thu, 12 Dec 2024 15:18:37 +0530 Subject: [PATCH 2/3] Address review comment --- .../apache/druid/indexing/common/task/HadoopTask.java | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java index ed0826e60570..a15b77b90d18 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java @@ -173,14 +173,7 @@ public static ClassLoader buildClassLoader(final List hadoopDependencyCo localClassLoaderURLs.addAll(Arrays.asList(hadoopLoader.getURLs())); } - ClassLoader parent; - try { - // See also https://docs.oracle.com/en/java/javase/11/migrate/index.html#JSMIG-GUID-A868D0B9-026F-4D46-B979-901834343F9E - parent = (ClassLoader) ClassLoader.class.getMethod("getPlatformClassLoader").invoke(null); - } - catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e) { - throw new RuntimeException(e); - } + ClassLoader parent = ClassLoader.getPlatformClassLoader(); final ClassLoader classLoader = new URLClassLoader( localClassLoaderURLs.toArray(new URL[0]), parent From d0475edcc9fce1d595cabc4c6767fd67c81a95a2 Mon Sep 17 00:00:00 2001 From: Akshat Jain Date: Thu, 12 Dec 2024 16:02:33 +0530 Subject: [PATCH 3/3] Ignore java.lang.ClassLoader for animal-sniffer check --- pom.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pom.xml b/pom.xml index 3ee7600d5437..3f80196334aa 100644 --- a/pom.xml +++ b/pom.xml @@ -1666,6 +1666,9 @@ sun.misc.Unsafe java.lang.invoke.MethodHandle + + java.lang.ClassLoader