OSDN Git Service

Don't mutate input in OSMemory.setIntArray/setShortArray.
authorElliott Hughes <enh@google.com>
Wed, 26 Aug 2009 16:37:48 +0000 (09:37 -0700)
committerElliott Hughes <enh@google.com>
Fri, 28 Aug 2009 01:05:55 +0000 (18:05 -0700)
We now take a copy and then swap bytes in the copy if necessary,
rather than swapping the input, copying, and swapping the input
back.

I've switched to GetShortArrayRegion/GetIntArrayRegion instead
of GetPrimitiveArrayCritical because the latter makes life
unnecessarily hard for the VM/GC, and requires a second JNI call
to undo.

I've also renamed the native functions to match the Java methods
they implement.

(Tested by running the nio tests on the emulator, with added
logging to check that all four variants are called.)

Bug: 2019584

libcore/luni/src/main/native/org_apache_harmony_luni_platform_OSMemory.cpp
libcore/luni/src/test/java/org/apache/harmony/luni/platform/AllTests.java [new file with mode: 0644]
libcore/luni/src/test/java/org/apache/harmony/luni/platform/OSMemoryTest.java [new file with mode: 0644]
libcore/luni/src/test/java/tests/AllTests.java

index 3e31743..2e814cc 100644 (file)
@@ -172,12 +172,10 @@ static void harmony_nio_putBytesImpl(JNIEnv *_env, jobject _this,
 }
 
 static void
-swapShorts(jshort *shorts, int numBytes) {
+swapShorts(jshort *shorts, int count) {
     jbyte *src = (jbyte *) shorts;
     jbyte *dst = src;
-    int i;
-    
-    for (i = 0; i < numBytes; i+=2) {
+    for (int i = 0; i < count; ++i) {
         jbyte b0 = *src++;
         jbyte b1 = *src++;
         *dst++ = b1;
@@ -186,11 +184,10 @@ swapShorts(jshort *shorts, int numBytes) {
 }
 
 static void
-swapInts(jint *ints, int numBytes) {
+swapInts(jint *ints, int count) {
     jbyte *src = (jbyte *) ints;
     jbyte *dst = src;
-    int i;   
-    for (i = 0; i < numBytes; i+=4) {
+    for (int i = 0; i < count; ++i) {
         jbyte b0 = *src++;
         jbyte b1 = *src++;
         jbyte b2 = *src++;
@@ -204,48 +201,30 @@ swapInts(jint *ints, int numBytes) {
 
 /*
  * Class:     org_apache_harmony_luni_platform_OSMemory
- * Method:    putShortsImpl
+ * Method:    setShortArrayImpl
  * Signature: (I[SIIZ)V
  */
-static void harmony_nio_putShortsImpl(JNIEnv *_env, jobject _this,
+static void harmony_nio_setShortArrayImpl(JNIEnv *_env, jobject _this,
        jint pointer, jshortArray src, jint offset, jint length, jboolean swap) {
-       
-    offset = offset << 1;
-    length = length << 1;
-       
-    jshort *src_ =
-        (jshort *)_env->GetPrimitiveArrayCritical(src, (jboolean *)0);
-    if (swap) {
-        swapShorts(src_ + offset, length);
-    }
-    memcpy((jbyte *)pointer, (jbyte *)src_ + offset, length);
+    jshort* dst = reinterpret_cast<jshort*>(static_cast<uintptr_t>(pointer));
+    _env->GetShortArrayRegion(src, offset, length, dst);
     if (swap) {
-        swapShorts(src_ + offset, length);
+        swapShorts(dst, length);
     }
-    _env->ReleasePrimitiveArrayCritical(src, src_, JNI_ABORT);
 }
 
 /*
  * Class:     org_apache_harmony_luni_platform_OSMemory
- * Method:    putIntsImpl
+ * Method:    setIntArrayImpl
  * Signature: (I[IIIZ)V
  */
-static void harmony_nio_putIntsImpl(JNIEnv *_env, jobject _this,
+static void harmony_nio_setIntArrayImpl(JNIEnv *_env, jobject _this,
        jint pointer, jintArray src, jint offset, jint length, jboolean swap) {
-
-    offset = offset << 2;
-    length = length << 2;
-       
-    jint *src_ =
-        (jint *)_env->GetPrimitiveArrayCritical(src, (jboolean *)0);
+    jint* dst = reinterpret_cast<jint*>(static_cast<uintptr_t>(pointer));
+    _env->GetIntArrayRegion(src, offset, length, dst);
     if (swap) {
-        swapInts(src_ + offset, length);
+        swapInts(dst, length);
     }
-    memcpy((jbyte *)pointer, (jbyte *)src_ + offset, length);
-    if (swap) {
-        swapInts(src_ + offset, length);
-    }
-    _env->ReleasePrimitiveArrayCritical(src, src_, JNI_ABORT);
 }
 
 /*
@@ -588,8 +567,8 @@ static JNINativeMethod gMethods[] = {
     { "memmove",            "(IIJ)V",  (void*) harmony_nio_memmove },
     { "getByteArray",       "(I[BII)V",(void*) harmony_nio_getBytesImpl },
     { "setByteArray",       "(I[BII)V",(void*) harmony_nio_putBytesImpl },
-    { "setShortArray",     "(I[SIIZ)V",(void*) harmony_nio_putShortsImpl },
-    { "setIntArray",       "(I[IIIZ)V",(void*) harmony_nio_putIntsImpl },
+    { "setShortArray",     "(I[SIIZ)V",(void*) harmony_nio_setShortArrayImpl },
+    { "setIntArray",       "(I[IIIZ)V",(void*) harmony_nio_setIntArrayImpl },
     { "getByte",            "(I)B",    (void*) harmony_nio_getByteImpl },
     { "setByte",            "(IB)V",   (void*) harmony_nio_putByteImpl },
     { "getShort",           "(I)S",    (void*) harmony_nio_getShortImpl },
@@ -653,4 +632,3 @@ int register_org_apache_harmony_luni_platform_OSMemory(JNIEnv *_env) {
                 "org/apache/harmony/luni/platform/OSMemory",
                 gMethods, NELEM(gMethods));
 }
-
diff --git a/libcore/luni/src/test/java/org/apache/harmony/luni/platform/AllTests.java b/libcore/luni/src/test/java/org/apache/harmony/luni/platform/AllTests.java
new file mode 100644 (file)
index 0000000..be28d41
--- /dev/null
@@ -0,0 +1,36 @@
+/*
+ * 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.harmony.luni.platform;
+
+import junit.framework.Test;
+import junit.framework.TestSuite;
+import junit.textui.TestRunner;
+
+public class AllTests {
+    public static void run() {
+        TestRunner.main(new String[] { AllTests.class.getName() });
+    }
+    
+    public static final Test suite() {
+        TestSuite suite = tests.TestSuiteFactory.createTestSuite("Tests for org.apache.harmony.luni.platform");
+        
+        suite.addTestSuite(OSMemoryTest.class);
+        
+        return suite;
+    }
+}
diff --git a/libcore/luni/src/test/java/org/apache/harmony/luni/platform/OSMemoryTest.java b/libcore/luni/src/test/java/org/apache/harmony/luni/platform/OSMemoryTest.java
new file mode 100644 (file)
index 0000000..a546289
--- /dev/null
@@ -0,0 +1,164 @@
+/* 
+ * 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.harmony.luni.platform;
+
+import dalvik.annotation.TestLevel;
+import dalvik.annotation.TestTargetNew;
+import dalvik.annotation.TestTargetClass;
+import dalvik.annotation.AndroidOnly;
+
+import junit.framework.TestCase;
+
+/**
+ * Tests org.apache.harmony.luni.platform.OSMemory (via IMemorySystem).
+ */
+@TestTargetClass(org.apache.harmony.luni.platform.OSMemory.class)
+public class OSMemoryTest extends TestCase {
+    @TestTargetNew(
+        level = TestLevel.PARTIAL_COMPLETE,
+        notes = "",
+        method = "memset",
+        args = {}
+    )
+    public void testMemset() {
+        IMemorySystem memory = Platform.getMemorySystem();
+        
+        int byteCount = 32;
+        int ptr = memory.malloc(byteCount);
+        try {
+            // Ensure our newly-allocated block isn't zeroed.
+            memory.setByte(ptr, (byte) 1);
+            assertEquals((byte) 1, memory.getByte(ptr));
+            // Check that we can clear memory.
+            memory.memset(ptr, (byte) 0, byteCount);
+            assertBytesEqual((byte) 0, ptr, byteCount);
+            // Check that we can set an arbitrary value.
+            memory.memset(ptr, (byte) 1, byteCount);
+            assertBytesEqual((byte) 1, ptr, byteCount);
+        } finally {
+            memory.free(ptr);
+        }
+    }
+    
+    void assertBytesEqual(byte value, int ptr, int byteCount) {
+        IMemorySystem memory = Platform.getMemorySystem();
+        for (int i = 0; i < byteCount; ++i) {
+            assertEquals(value, memory.getByte(ptr + i));
+        }
+    }
+    
+    @TestTargetNew(
+        level = TestLevel.PARTIAL_COMPLETE,
+        notes = "",
+        method = "setIntArray",
+        args = {}
+    )
+    public void testSetIntArray() {
+        IMemorySystem memory = Platform.getMemorySystem();
+        
+        int[] values = { 3, 7, 31, 127, 8191, 131071, 524287, 2147483647 };
+        int[] swappedValues = new int[values.length];
+        for (int i = 0; i < values.length; ++i) {
+            swappedValues[i] = swapInt(values[i]);
+        }
+        
+        int scale = ICommonDataTypes.SIZEOF_JINT;
+        int ptr = memory.malloc(scale * values.length);
+        try {
+            // Regular copy. Memset first so we start from a known state.
+            memory.memset(ptr, (byte) 0, scale * values.length);
+            memory.setIntArray(ptr, values, 0, values.length, false);
+            assertIntsEqual(values, ptr);
+            
+            // Swapped copy.
+            memory.memset(ptr, (byte) 0, scale * values.length);
+            memory.setIntArray(ptr, values, 0, values.length, true);
+            assertIntsEqual(swappedValues, ptr);
+            
+            // Swapped copies of slices (to ensure we test non-zero offsets).
+            memory.memset(ptr, (byte) 0, scale * values.length);
+            for (int i = 0; i < values.length; ++i) {
+                memory.setIntArray(ptr + i * scale, values, i, 1, true);
+            }
+            assertIntsEqual(swappedValues, ptr);
+        } finally {
+            memory.free(ptr);
+        }
+    }
+    
+    private void assertIntsEqual(int[] expectedValues, int ptr) {
+        IMemorySystem memory = Platform.getMemorySystem();
+        for (int i = 0; i < expectedValues.length; ++i) {
+            assertEquals(expectedValues[i], memory.getInt(ptr + ICommonDataTypes.SIZEOF_JINT * i));
+        }
+    }
+    
+    private static int swapInt(int n) {
+        return (((n >>  0) & 0xff) << 24) |
+               (((n >>  8) & 0xff) << 16) |
+               (((n >> 16) & 0xff) <<  8) |
+               (((n >> 24) & 0xff) <<  0);
+    }
+    
+    @TestTargetNew(
+        level = TestLevel.PARTIAL_COMPLETE,
+        notes = "",
+        method = "setShortArray",
+        args = {}
+    )
+    public void testSetShortArray() {
+        IMemorySystem memory = Platform.getMemorySystem();
+        
+        short[] values = { 0x0001, 0x0020, 0x0300, 0x4000 };
+        short[] swappedValues = { 0x0100, 0x2000, 0x0003, 0x0040 };
+        
+        int scale = ICommonDataTypes.SIZEOF_JSHORT;
+        int ptr = memory.malloc(scale * values.length);
+        try {
+            // Regular copy. Memset first so we start from a known state.
+            memory.memset(ptr, (byte) 0, scale * values.length);
+            memory.setShortArray(ptr, values, 0, values.length, false);
+            assertShortsEqual(values, ptr);
+            
+            // Swapped copy.
+            memory.memset(ptr, (byte) 0, scale * values.length);
+            memory.setShortArray(ptr, values, 0, values.length, true);
+            assertShortsEqual(swappedValues, ptr);
+            
+            // Swapped copies of slices (to ensure we test non-zero offsets).
+            memory.memset(ptr, (byte) 0, scale * values.length);
+            for (int i = 0; i < values.length; ++i) {
+                memory.setShortArray(ptr + i * scale, values, i, 1, true);
+            }
+            assertShortsEqual(swappedValues, ptr);
+        } finally {
+            memory.free(ptr);
+        }
+    }
+    
+    private void assertShortsEqual(short[] expectedValues, int ptr) {
+        IMemorySystem memory = Platform.getMemorySystem();
+        for (int i = 0; i < expectedValues.length; ++i) {
+            assertEquals(expectedValues[i], memory.getShort(ptr + ICommonDataTypes.SIZEOF_JSHORT * i));
+        }
+    }
+    
+    private static short swapShort(short n) {
+        return (short) ((((n >>  0) & 0xff) << 8) | (((n >>  8) & 0xff) << 0));
+    }
+}
index 26e58c1..893cdf0 100644 (file)
@@ -54,6 +54,8 @@ public class AllTests
         suite.addTest(tests.xml.AllTests.suite());
         suite.addTest(tests.xnet.AllTests.suite());
 
+        suite.addTest(org.apache.harmony.luni.platform.AllTests.suite());
+        
         return suite;
     }
 }