From f89636d49dfc33252034d16d15f59f551db56e50 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Wed, 14 Sep 2016 09:52:06 -0700 Subject: [PATCH 1/5] Remove reflection in ByteBufferUtils * Also adds test --- .../com/metamx/common/ByteBufferUtils.java | 56 ++++++---------- .../metamx/common/ByteBufferUtilsTest.java | 67 +++++++++++++++++++ 2 files changed, 86 insertions(+), 37 deletions(-) create mode 100644 src/test/java/com/metamx/common/ByteBufferUtilsTest.java diff --git a/src/main/java/com/metamx/common/ByteBufferUtils.java b/src/main/java/com/metamx/common/ByteBufferUtils.java index f332d89c..57641252 100644 --- a/src/main/java/com/metamx/common/ByteBufferUtils.java +++ b/src/main/java/com/metamx/common/ByteBufferUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2011,2012 Metamarkets Group Inc. + * Copyright 2011-2016 Metamarkets Group Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,9 +17,9 @@ package com.metamx.common; import com.google.common.base.Throwables; +import sun.misc.Cleaner; +import sun.nio.ch.DirectBuffer; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; import java.nio.ByteBuffer; import java.nio.MappedByteBuffer; @@ -27,45 +27,19 @@ */ public class ByteBufferUtils { - private static final Method unmap; - - private static final Method getCleaner; - private static final Method clean; - - - static { - try { - Method unmapMethod = Class.forName("sun.nio.ch.FileChannelImpl") - .getDeclaredMethod("unmap", MappedByteBuffer.class); - unmapMethod.setAccessible(true); - unmap = unmapMethod; - } - catch (Exception e) { - throw new UOE(e, "Exception thrown while trying to find unmap method on MappedByteBuffer, " - + "this method must exist in your VM in order for this to work"); - } - } - - static { - try { - getCleaner = Class.forName("java.nio.DirectByteBuffer").getDeclaredMethod("cleaner"); - getCleaner.setAccessible(true); - clean = Class.forName("sun.misc.Cleaner").getDeclaredMethod("clean"); - clean.setAccessible(true); - } catch(ClassNotFoundException | NoSuchMethodException e) { - throw new UOE("Exception thrown while trying to access ByteBuffer clean method."); - } - } - /** * Releases memory held by the given direct ByteBuffer * * @param buffer buffer to free */ - public static void free(ByteBuffer buffer) { + public static void free(ByteBuffer buffer) + { try { - clean.invoke(getCleaner.invoke(buffer)); - } catch(IllegalAccessException | InvocationTargetException e) { + if (buffer.isDirect()) { + clean((DirectBuffer) buffer); + } + } + catch (Exception e) { throw Throwables.propagate(e); } } @@ -79,10 +53,18 @@ public static void free(ByteBuffer buffer) { public static void unmap(MappedByteBuffer buffer) { try { - unmap.invoke(null, buffer); + clean((DirectBuffer) buffer); } catch (Exception e) { throw Throwables.propagate(e); } } + + private static void clean(DirectBuffer buffer) + { + final Cleaner cleaner = buffer.cleaner(); + if (cleaner != null) { + cleaner.clean(); + } + } } diff --git a/src/test/java/com/metamx/common/ByteBufferUtilsTest.java b/src/test/java/com/metamx/common/ByteBufferUtilsTest.java new file mode 100644 index 00000000..4954ea40 --- /dev/null +++ b/src/test/java/com/metamx/common/ByteBufferUtilsTest.java @@ -0,0 +1,67 @@ +/* + * Copyright 2016 Metamarkets Group Inc. + * + * 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.metamx.common; + +import junit.framework.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.BufferedOutputStream; +import java.io.File; +import java.io.FileOutputStream; +import java.io.OutputStream; +import java.nio.ByteBuffer; +import java.nio.MappedByteBuffer; +import java.nio.channels.FileChannel; +import java.nio.file.StandardOpenOption; +import java.util.Arrays; + +public class ByteBufferUtilsTest +{ + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Test + public void testUnmapDoesntCrashJVM() throws Exception + { + final File file = temporaryFolder.newFile("some_mmap_file"); + try (final OutputStream os = new BufferedOutputStream(new FileOutputStream(file))) { + final byte[] data = new byte[4096]; + Arrays.fill(data, (byte) 0x5A); + os.write(data); + } + final MappedByteBuffer mappedByteBuffer; + try (final FileChannel channel = FileChannel.open(file.toPath(), StandardOpenOption.READ)) { + mappedByteBuffer = channel.map(FileChannel.MapMode.READ_ONLY, 0, 4096); + } + Assert.assertEquals((byte) 0x5A, mappedByteBuffer.get(0)); + ByteBufferUtils.unmap(mappedByteBuffer); + ByteBufferUtils.unmap(mappedByteBuffer); + } + + @Test + public void testFreeDoesntCrashJVM() throws Exception + { + final ByteBuffer directBuffer = ByteBuffer.allocateDirect(4096); + ByteBufferUtils.free(directBuffer); + ByteBufferUtils.free(directBuffer); + + final ByteBuffer heapBuffer = ByteBuffer.allocate(4096); + ByteBufferUtils.free(heapBuffer); + } +} \ No newline at end of file From 3aed7dad665e04508b2f98fe3b531f1ec0c76bd9 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Wed, 14 Sep 2016 09:54:42 -0700 Subject: [PATCH 2/5] Add missing newline --- src/test/java/com/metamx/common/ByteBufferUtilsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/metamx/common/ByteBufferUtilsTest.java b/src/test/java/com/metamx/common/ByteBufferUtilsTest.java index 4954ea40..3ad5238a 100644 --- a/src/test/java/com/metamx/common/ByteBufferUtilsTest.java +++ b/src/test/java/com/metamx/common/ByteBufferUtilsTest.java @@ -64,4 +64,4 @@ public void testFreeDoesntCrashJVM() throws Exception final ByteBuffer heapBuffer = ByteBuffer.allocate(4096); ByteBufferUtils.free(heapBuffer); } -} \ No newline at end of file +} From f759ef7864122c33d64fa9df80619a386a409e93 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Wed, 14 Sep 2016 09:58:59 -0700 Subject: [PATCH 3/5] Make `unmap` just call `free` --- src/main/java/com/metamx/common/ByteBufferUtils.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/main/java/com/metamx/common/ByteBufferUtils.java b/src/main/java/com/metamx/common/ByteBufferUtils.java index 57641252..3ed3defd 100644 --- a/src/main/java/com/metamx/common/ByteBufferUtils.java +++ b/src/main/java/com/metamx/common/ByteBufferUtils.java @@ -52,12 +52,7 @@ public static void free(ByteBuffer buffer) */ public static void unmap(MappedByteBuffer buffer) { - try { - clean((DirectBuffer) buffer); - } - catch (Exception e) { - throw Throwables.propagate(e); - } + free(buffer); } private static void clean(DirectBuffer buffer) From 0d34f86d76722bf9c00507e268799a06af8d1546 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Wed, 14 Sep 2016 13:32:34 -0700 Subject: [PATCH 4/5] Cleaner test code through the magic of Guava --- src/test/java/com/metamx/common/ByteBufferUtilsTest.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/test/java/com/metamx/common/ByteBufferUtilsTest.java b/src/test/java/com/metamx/common/ByteBufferUtilsTest.java index 3ad5238a..d83025ea 100644 --- a/src/test/java/com/metamx/common/ByteBufferUtilsTest.java +++ b/src/test/java/com/metamx/common/ByteBufferUtilsTest.java @@ -16,6 +16,7 @@ package com.metamx.common; +import com.google.common.io.Files; import junit.framework.Assert; import org.junit.Rule; import org.junit.Test; @@ -27,8 +28,6 @@ import java.io.OutputStream; import java.nio.ByteBuffer; import java.nio.MappedByteBuffer; -import java.nio.channels.FileChannel; -import java.nio.file.StandardOpenOption; import java.util.Arrays; public class ByteBufferUtilsTest @@ -45,10 +44,7 @@ public void testUnmapDoesntCrashJVM() throws Exception Arrays.fill(data, (byte) 0x5A); os.write(data); } - final MappedByteBuffer mappedByteBuffer; - try (final FileChannel channel = FileChannel.open(file.toPath(), StandardOpenOption.READ)) { - mappedByteBuffer = channel.map(FileChannel.MapMode.READ_ONLY, 0, 4096); - } + final MappedByteBuffer mappedByteBuffer = Files.map(file); Assert.assertEquals((byte) 0x5A, mappedByteBuffer.get(0)); ByteBufferUtils.unmap(mappedByteBuffer); ByteBufferUtils.unmap(mappedByteBuffer); From 91d0d6fd2aa0402376466d3870c27f54270a6e9b Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Wed, 14 Sep 2016 13:33:40 -0700 Subject: [PATCH 5/5] Remove supurfluous try block --- src/main/java/com/metamx/common/ByteBufferUtils.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/metamx/common/ByteBufferUtils.java b/src/main/java/com/metamx/common/ByteBufferUtils.java index 3ed3defd..e81275c8 100644 --- a/src/main/java/com/metamx/common/ByteBufferUtils.java +++ b/src/main/java/com/metamx/common/ByteBufferUtils.java @@ -16,7 +16,6 @@ package com.metamx.common; -import com.google.common.base.Throwables; import sun.misc.Cleaner; import sun.nio.ch.DirectBuffer; @@ -34,13 +33,8 @@ public class ByteBufferUtils */ public static void free(ByteBuffer buffer) { - try { - if (buffer.isDirect()) { - clean((DirectBuffer) buffer); - } - } - catch (Exception e) { - throw Throwables.propagate(e); + if (buffer.isDirect()) { + clean((DirectBuffer) buffer); } }