You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2021/05/11 20:43:19 UTC
[GitHub] [datasketches-memory] davecromberge commented on a change in pull request #128: No split package
davecromberge commented on a change in pull request #128:
URL: https://github.com/apache/datasketches-memory/pull/128#discussion_r630512286
##########
File path: src/main/java/org/apache/datasketches/memory/Util.java
##########
@@ -34,10 +35,50 @@
* @author Lee Rhodes
*/
public final class Util {
- private static final String LS = System.getProperty("line.separator");
+ public static final String LS = System.getProperty("line.separator");
+
+ //Byte Order related
+ public static final ByteOrder nativeByteOrder = ByteOrder.nativeOrder();
+ public static final ByteOrder nonNativeByteOrder = nativeByteOrder == ByteOrder.LITTLE_ENDIAN
+ ? ByteOrder.BIG_ENDIAN : ByteOrder.LITTLE_ENDIAN;
+ /**
+ * Don't use sun.misc.Unsafe#copyMemory to copy blocks of memory larger than this
+ * threshold, because internally it doesn't have safepoint polls, that may cause long
+ * "Time To Safe Point" pauses in the application. This has been fixed in JDK 9 (see
+ * https://bugs.openjdk.java.net/browse/JDK-8149596 and
+ * https://bugs.openjdk.java.net/browse/JDK-8141491), but not in JDK 8, so the Memory library
+ * should keep having this boilerplate as long as it supports Java 8.
+ *
+ * <p>A reference to this can be found in java.nio.Bits.</p>
+ */
+ public static final int UNSAFE_COPY_THRESHOLD_BYTES = 1024 * 1024;
Review comment:
Is it safe to exceed this threshold in JVM versions 9+, since long time to safepoints would have been fixed? Is this a constant that can move into the multirelease manifest and be redefined to something larger?
##########
File path: src/test/java/org/apache/datasketches/memory/test/AllocateDirectWritableMapMemoryTest.java
##########
@@ -21,11 +21,11 @@
* Note: Lincoln's Gettysburg Address is in the public domain. See LICENSE.
*/
-package org.apache.datasketches.memory;
+package org.apache.datasketches.memory.test;
import static java.nio.charset.StandardCharsets.UTF_8;
-import static org.apache.datasketches.memory.AllocateDirectMap.isFileReadOnly;
-import static org.apache.datasketches.memory.Util.getResourceFile;
+//import static org.apache.datasketches.memory.AllocateDirectMap.isFileReadOnly;
Review comment:
Nit - the import on line 27 can be removed
##########
File path: src/main/java/org/apache/datasketches/memory/Prim.java
##########
@@ -52,7 +52,7 @@
*
* @author Lee Rhodes
*/
-enum Prim {
+public enum Prim {
Review comment:
Is `Prim` an abbreviation for `Primitive`? Does it encapsulate offsets for different primitive types?
##########
File path: src/test/java/org/apache/datasketches/memory/test/AllocateDirectMemoryTest.java
##########
@@ -17,35 +17,84 @@
* under the License.
*/
-package org.apache.datasketches.memory;
+package org.apache.datasketches.memory.test;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.fail;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.nio.ByteOrder;
+
+import org.apache.datasketches.memory.MemoryRequestServer;
+import org.apache.datasketches.memory.Util;
+import org.apache.datasketches.memory.WritableDirectHandle;
+import org.apache.datasketches.memory.WritableHandle;
+import org.apache.datasketches.memory.WritableMemory;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;
@SuppressWarnings("javadoc")
public class AllocateDirectMemoryTest {
-
+
+ static final Method CHECK_VALID;
+ static final Method WRAP_DIRECT;
+ static final Method GET_CURRENT_DIRECT_MEMORY_ALLOCATIONS;
+
+ static {
+ CHECK_VALID =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "checkValid", (Class<?>[])null); //not static
+ WRAP_DIRECT =
+ ReflectUtil.getMethod(ReflectUtil.BASE_WRITABLE_MEMORY_IMPL,
+ "wrapDirect", long.class, ByteOrder.class, MemoryRequestServer.class); //static method
+ GET_CURRENT_DIRECT_MEMORY_ALLOCATIONS =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE,
+ "getCurrentDirectMemoryAllocations", (Class<?>[])null); //static method
+ }
+
+ private static long getCurrentDirectMemoryAllocations() {
+ try {
+ return (long) GET_CURRENT_DIRECT_MEMORY_ALLOCATIONS.invoke(null);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static WritableDirectHandle wrapDirect(final long capacityBytes,
+ final ByteOrder byteOrder, final MemoryRequestServer memReqSvr) {
+ try {
+ return (WritableDirectHandle) WRAP_DIRECT.invoke(null, capacityBytes, byteOrder, memReqSvr);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
@Test
public void simpleAllocateDirect() {
int longs = 32;
- WritableMemory wMem1;
+ WritableMemory wMem;
try (WritableHandle wh = WritableMemory.allocateDirect(longs << 3)) {
- wMem1 = wh.get();
+ wMem = wh.get();
for (int i = 0; i<longs; i++) {
- wMem1.putLong(i << 3, i);
- assertEquals(wMem1.getLong(i << 3), i);
+ wMem.putLong(i << 3, i);
+ assertEquals(wMem.getLong(i << 3), i);
+ }
+ //inside the TWR block the memory should be valid
Review comment:
What is a TWR block?
##########
File path: src/main/java/org/apache/datasketches/memory/StepBoolean.java
##########
@@ -62,7 +62,7 @@ boolean change() {
* Return true if the state has changed from the initial state
* @return true if the state has changed from the initial state
*/
- boolean hasChanged() {
+ public boolean hasChanged() {
return state != initialState;
}
Review comment:
Did you add the public visibility modifier for reflective access from the tests?
##########
File path: src/test/java/org/apache/datasketches/memory/test/AllocateDirectWritableMapMemoryTest.java
##########
@@ -45,6 +53,28 @@
public class AllocateDirectWritableMapMemoryTest {
private static final String LS = System.getProperty("line.separator");
+ static final Method IS_FILE_READ_ONLY;
+ static final Method GET_CURRENT_DIRECT_MEMORY_MAP_ALLOCATIONS;
+
+ static {
+ IS_FILE_READ_ONLY =
+ ReflectUtil.getMethod(ReflectUtil.ALLOCATE_DIRECT_MAP, "isFileReadOnly", File.class);
+ GET_CURRENT_DIRECT_MEMORY_MAP_ALLOCATIONS =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "getCurrentDirectMemoryMapAllocations", (Class<?>[])null); //static
+ }
+
+ private static boolean isFileReadOnly(final File file) {
+ try {
+ return (boolean) IS_FILE_READ_ONLY.invoke(null, file);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static long getCurrentDirectMemoryMapAllocations() {
+ try {
+ return (long) GET_CURRENT_DIRECT_MEMORY_MAP_ALLOCATIONS.invoke(null);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
Review comment:
The previous strategy imported static methods into the test suite - is the new strategy to redefine them locally and delegate to the actual static method using reflection? This has minimal impact on the existing tests and is a nice approach.
##########
File path: src/test/java/org/apache/datasketches/memory/test/MemoryTest.java
##########
@@ -383,17 +430,17 @@ public void checkMonitorDirectStats() {
int bytes = 1024;
WritableHandle wh1 = WritableMemory.allocateDirect(bytes);
WritableHandle wh2 = WritableMemory.allocateDirect(bytes);
- assertEquals(BaseState.getCurrentDirectMemoryAllocations(), 2L);
- assertEquals(BaseState.getCurrentDirectMemoryAllocated(), 2 * bytes);
+ assertEquals(getCurrentDirectMemoryAllocations(), 2L);
+ assertEquals(getCurrentDirectMemoryAllocated(), 2 * bytes);
wh1.close();
- assertEquals(BaseState.getCurrentDirectMemoryAllocations(), 1L);
- assertEquals(BaseState.getCurrentDirectMemoryAllocated(), bytes);
+ assertEquals(getCurrentDirectMemoryAllocations(), 1L);
+ assertEquals(getCurrentDirectMemoryAllocated(), bytes);
wh2.close();
wh2.close(); //check that it doesn't go negative.
- assertEquals(BaseState.getCurrentDirectMemoryAllocations(), 0L);
- assertEquals(BaseState.getCurrentDirectMemoryAllocated(), 0L);
+ assertEquals(getCurrentDirectMemoryAllocations(), 0L);
+ assertEquals(getCurrentDirectMemoryAllocated(), 0L);
Review comment:
Do you allocate the direct memory through the public API and make assertions about the state number of allocations and allocated bytes through reflection? This is an important distinction that I need to understand - as earlier in the PR you made many static methods public.
##########
File path: src/test/java/org/apache/datasketches/memory/test/AllocateDirectMemoryTest.java
##########
@@ -17,35 +17,84 @@
* under the License.
*/
-package org.apache.datasketches.memory;
+package org.apache.datasketches.memory.test;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.fail;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.nio.ByteOrder;
+
+import org.apache.datasketches.memory.MemoryRequestServer;
+import org.apache.datasketches.memory.Util;
+import org.apache.datasketches.memory.WritableDirectHandle;
+import org.apache.datasketches.memory.WritableHandle;
+import org.apache.datasketches.memory.WritableMemory;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;
@SuppressWarnings("javadoc")
public class AllocateDirectMemoryTest {
-
+
+ static final Method CHECK_VALID;
+ static final Method WRAP_DIRECT;
+ static final Method GET_CURRENT_DIRECT_MEMORY_ALLOCATIONS;
+
+ static {
+ CHECK_VALID =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "checkValid", (Class<?>[])null); //not static
+ WRAP_DIRECT =
+ ReflectUtil.getMethod(ReflectUtil.BASE_WRITABLE_MEMORY_IMPL,
+ "wrapDirect", long.class, ByteOrder.class, MemoryRequestServer.class); //static method
Review comment:
The comments are a little misleading - did you mean to say that line 48 _is_ static and line 51 is _not_ a static method?
##########
File path: src/test/java/org/apache/datasketches/memory/test/SpecificLeafTest.java
##########
@@ -0,0 +1,278 @@
+/*
+ * 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.datasketches.memory.test;
+
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+import java.lang.reflect.Method;
+import java.nio.ByteBuffer;
+
+import org.apache.datasketches.memory.Buffer;
+import org.apache.datasketches.memory.Memory;
+import org.apache.datasketches.memory.Util;
+import org.apache.datasketches.memory.WritableDirectHandle;
+import org.apache.datasketches.memory.WritableMapHandle;
+import org.apache.datasketches.memory.WritableMemory;
+import org.testng.annotations.Test;
+
+/**
+ * @author Lee Rhodes
+ */
+@SuppressWarnings("javadoc")
+public class SpecificLeafTest {
+
+ static final Method IS_READ_ONLY_TYPE;
+ static final Method IS_BUFFER_TYPE;
+ static final Method IS_DUPLICATE_TYPE;
+ static final Method IS_REGION_TYPE;
+ static final Method IS_NON_NATIVE_TYPE;
+ static final Method IS_HEAP_TYPE;
+ static final Method IS_DIRECT_TYPE;
+ static final Method IS_MAP_TYPE;
+ static final Method IS_BB_TYPE;
+
+ static {
+ IS_READ_ONLY_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isReadOnlyType", (Class<?>[])null); //not static
+ IS_BUFFER_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isBufferType", (Class<?>[])null); //not static
+ IS_DUPLICATE_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isDuplicateType", (Class<?>[])null); //not static
+ IS_REGION_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isRegionType", (Class<?>[])null); //not static
+ IS_NON_NATIVE_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isNonNativeType", (Class<?>[])null); //not static
+ IS_HEAP_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isHeapType", (Class<?>[])null); //not static
+ IS_DIRECT_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isDirectType", (Class<?>[])null); //not static
+ IS_MAP_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isMapType", (Class<?>[])null); //not static
+ IS_BB_TYPE =
+ ReflectUtil.getMethod(ReflectUtil.BASE_STATE, "isBBType", (Class<?>[])null); //not static
+ }
+
+ private static boolean isReadOnlyType(final Object owner) {
+ try {
+ return (boolean) IS_READ_ONLY_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isBufferType(final Object owner) {
+ try {
+ return (boolean) IS_BUFFER_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isDuplicateType(final Object owner) {
+ try {
+ return (boolean) IS_DUPLICATE_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isRegionType(final Object owner) {
+ try {
+ return (boolean) IS_REGION_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isNonNativeType(final Object owner) {
+ try {
+ return (boolean) IS_NON_NATIVE_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isHeapType(final Object owner) {
+ try {
+ return (boolean) IS_HEAP_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isDirectType(final Object owner) {
+ try {
+ return (boolean) IS_DIRECT_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isMapType(final Object owner) {
+ try {
+ return (boolean) IS_MAP_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+ private static boolean isBBType(final Object owner) {
+ try {
+ return (boolean) IS_BB_TYPE.invoke(owner);
+ } catch (Exception e) { throw new RuntimeException(e); }
+ }
+
+
+
+ @Test
+ public void checkByteBufferLeafs() {
+ int bytes = 128;
+ ByteBuffer bb = ByteBuffer.allocate(bytes);
+ bb.order(Util.nativeByteOrder);
+
+ Memory mem = Memory.wrap(bb).region(0, bytes, Util.nativeByteOrder);
+ assertTrue(isBBType(mem));
+ assertTrue(mem.isReadOnly());
+ checkCrossLeafTypeIds(mem);
+ Buffer buf = mem.asBuffer().region(0, bytes, Util.nativeByteOrder);
+
+ bb.order(Util.nonNativeByteOrder);
+ Memory mem2 = Memory.wrap(bb).region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf2 = mem2.asBuffer().region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf3 = buf2.duplicate();
+
+ assertTrue(isRegionType(mem));
+ assertTrue(isRegionType(mem2));
+ assertTrue(isRegionType(buf));
+ assertTrue(isRegionType(buf2));
+ assertTrue(isDuplicateType(buf3));
+ }
+
+ @Test
+ public void checkDirectLeafs() {
+ int bytes = 128;
+ try (WritableDirectHandle h = WritableMemory.allocateDirect(bytes)) {
+ WritableMemory wmem = h.get(); //native mem
+ assertTrue(isDirectType(wmem));
+ assertFalse(wmem.isReadOnly());
+ checkCrossLeafTypeIds(wmem);
+ WritableMemory nnwmem = wmem.writableRegion(0, bytes, Util.nonNativeByteOrder);
+
+ Memory mem = wmem.region(0, bytes, Util.nativeByteOrder);
+ Buffer buf = mem.asBuffer().region(0, bytes, Util.nativeByteOrder);
+
+
+ Memory mem2 = nnwmem.region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf2 = mem2.asBuffer().region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf3 = buf2.duplicate();
+
+ assertTrue(isRegionType(mem));
+ assertTrue(isRegionType(mem2));
+ assertTrue(isRegionType(buf));
+ assertTrue(isRegionType(buf2));
+ assertTrue(isDuplicateType(buf3));
+ }
+ }
+
+ @Test
+ public void checkMapLeafs() throws IOException {
+ File file = new File("TestFile2.bin");
+ if (file.exists()) {
+ try {
+ java.nio.file.Files.delete(file.toPath());
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ assertTrue(file.createNewFile());
+ assertTrue(file.setWritable(true, false)); //writable=true, ownerOnly=false
+ assertTrue(file.isFile());
+ file.deleteOnExit(); //comment out if you want to examine the file.
+
+ final long bytes = 128;
+
+ try (WritableMapHandle h = WritableMemory.map(file, 0L, bytes, Util.nativeByteOrder)) {
+ WritableMemory mem = h.get(); //native mem
+ assertTrue(isMapType(mem));
+ assertFalse(mem.isReadOnly());
+ checkCrossLeafTypeIds(mem);
+ Memory nnreg = mem.region(0, bytes, Util.nonNativeByteOrder);
+
+ Memory reg = mem.region(0, bytes, Util.nativeByteOrder);
+ Buffer buf = reg.asBuffer().region(0, bytes, Util.nativeByteOrder);
+ Buffer buf4 = buf.duplicate();
+
+ Memory reg2 = nnreg.region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf2 = reg2.asBuffer().region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf3 = buf2.duplicate();
+
+ assertTrue(isRegionType(reg));
+ assertTrue(isRegionType(reg2));
+ assertTrue(isRegionType(buf));
+ assertTrue(isRegionType(buf2));
+ assertTrue(isDuplicateType(buf3));
+ assertTrue(isDuplicateType(buf4));
+ }
+ }
+
+ @Test
+ public void checkHeapLeafs() {
+ int bytes = 128;
+ Memory mem = Memory.wrap(new byte[bytes]);
+ assertTrue(isHeapType(mem));
+ assertTrue(isReadOnlyType(mem));
+ checkCrossLeafTypeIds(mem);
+ Memory nnreg = mem.region(0, bytes, Util.nonNativeByteOrder);
+
+ Memory reg = mem.region(0, bytes, Util.nativeByteOrder);
+ Buffer buf = reg.asBuffer().region(0, bytes, Util.nativeByteOrder);
+ Buffer buf4 = buf.duplicate();
+
+ Memory reg2 = nnreg.region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf2 = reg2.asBuffer().region(0, bytes, Util.nonNativeByteOrder);
+ Buffer buf3 = buf2.duplicate();
+
+ assertFalse(isRegionType(mem));
+ assertTrue(isRegionType(reg2));
+ assertTrue(isRegionType(buf));
+ assertTrue(isRegionType(buf2));
+ assertTrue(isDuplicateType(buf3));
+ assertTrue(isDuplicateType(buf4));
+ }
+
+ private static void checkCrossLeafTypeIds(Memory mem) {
+ Memory reg1 = mem.region(0, mem.getCapacity());
+ assertTrue(isRegionType(reg1));
+
+ Buffer buf1 = reg1.asBuffer();
+ assertTrue(isRegionType(buf1));
+ assertTrue(isBufferType(buf1));
+
+ Buffer buf2 = buf1.duplicate();
+ assertTrue(isRegionType(buf2));
+ assertTrue(isBufferType(buf2));
+ assertTrue(isDuplicateType(buf2));
+
+ Memory mem2 = buf1.asMemory();
+ assertTrue(isRegionType(mem2));
+ assertFalse(isBufferType(mem2));
+ assertFalse(isDuplicateType(mem2));
+
+ Buffer buf3 = buf1.duplicate(Util.nonNativeByteOrder);
+ assertTrue(isRegionType(buf3));
+ assertTrue(isBufferType(buf3));
+ assertTrue(isDuplicateType(buf3));
+ assertTrue(isNonNativeType(buf3));
+
+ Memory mem3 = buf3.asMemory();
+ assertTrue(isRegionType(mem3));
+ assertFalse(isBufferType(mem3));
+ assertFalse(isDuplicateType(mem3));
+ assertFalse(isNonNativeType(mem3));
+ }
+
+}
Review comment:
What is a leaf in the context of a byte buffer? Is the memory arranged hierarchically?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org