Re: svn commit: r950012 - in /harmony/enhanced/java/trunk/classlib/modules/nio/src: main/java/common/java/nio/ main/java/common/org/apache/harmony/nio/internal/ test/java/common/org/apache/harmony/nio/tests/java/nio/

[Oliver Deakin]

Hi all,

Just thought I'd give a heads up on this rearrangement of the class 
hierarchy of the ByteBuffer subclasses. The change was spurred on by the 
test case added in the same commit. Previously, the MappedByteBuffer 
class wrapped a subclass of DirectByteBuffer. Any method call on MBB 
would have to update the variables in the MBB instance and then make the 
same method call on the DBB to update its variables also. Unfortunately 
keeping these two sets of identical variables in sync was prone to error 
and, as the test case shows, we had bugs in a number of functions on MBB 
where the position/limit/etc. fields were not being updated correctly. 
Rearranging the class hierarchy simplifies these cases as it removes the 
duplication of variables between MBB and DBB instances. I think it also 
simplifies the code a little bit and removes a couple of classes we no 
longer need.

It is also interesting (but not a reason to make the change in itself) 
that the RI appears to have a similar hierarchy. The following code 
would execute on the RI but not on Harmony before my change:
    ByteBuffer directByteBuffer = ByteBuffer.allocateDirect(100);
    MappedByteBuffer mappedByteBuffer = (MappedByteBuffer)directByteBuffer;

Regards,
Oliver


On 01/06/2010 11:51, odea...@apache.org wrote:
Author: odeakin
Date: Tue Jun  1 10:51:17 2010
New Revision: 950012

URL: http://svn.apache.org/viewvc?rev=950012&view=rev
Log:
Rearrange class hierarchy for ByteBuffer subclasses - this removes the need to 
wrapper a DirectByteBuffer inside MappedByteBuffer and avoids having to keep 
two copies of the position/limit/etc. variables in sync in both classes, which 
was broken anyway as shown by the new test case added in this commit.

Removed:
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/BaseByteBuffer.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/MappedByteBufferAdapter.java
Modified:
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/DirectByteBuffer.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/HeapByteBuffer.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/MappedByteBuffer.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadOnlyDirectByteBuffer.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadWriteDirectByteBuffer.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/org/apache/harmony/nio/internal/MappedByteBufferFactory.java
     
harmony/enhanced/java/trunk/classlib/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/DirectByteBuffer.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/DirectByteBuffer.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/DirectByteBuffer.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/DirectByteBuffer.java
 Tue Jun  1 10:51:17 2010
@@ -34,10 +34,7 @@ import org.apache.harmony.nio.internal.n
   *</p>
   *
   */
-abstract class DirectByteBuffer extends BaseByteBuffer implements DirectBuffer 
{
-
-    // This is the base address of the buffer memory.
-    protected PlatformAddress address;
+abstract class DirectByteBuffer extends MappedByteBuffer implements 
DirectBuffer {

      // This is the offset from the base address at which this buffer logically
      // starts.
@@ -59,6 +56,7 @@ abstract class DirectByteBuffer extends
          super(capacity);
          this.address = address;
          this.offset = offset;
+        address.autoFree();
      }

      /*
@@ -275,4 +273,54 @@ abstract class DirectByteBuffer extends
      public final int getByteCapacity() {
          return capacity;
      }
+
+    @Override
+    public final CharBuffer asCharBuffer() {
+        return CharToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final DoubleBuffer asDoubleBuffer() {
+        return DoubleToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final FloatBuffer asFloatBuffer() {
+        return FloatToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final IntBuffer asIntBuffer() {
+        return IntToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final LongBuffer asLongBuffer() {
+        return LongToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final ShortBuffer asShortBuffer() {
+        return ShortToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final char getChar() {
+        return (char) getShort();
+    }
+
+    @Override
+    public final char getChar(int index) {
+        return (char) getShort(index);
+    }
+
+    @Override
+    public final ByteBuffer putChar(char value) {
+        return putShort((short) value);
+    }
+
+    @Override
+    public final ByteBuffer putChar(int index, char value) {
+        return putShort(index, (short) value);
+    }
  }

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/HeapByteBuffer.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/HeapByteBuffer.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/HeapByteBuffer.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/HeapByteBuffer.java
 Tue Jun  1 10:51:17 2010
@@ -31,7 +31,7 @@ import org.apache.harmony.luni.platform.
   *</p>
   *
   */
-abstract class HeapByteBuffer extends BaseByteBuffer {
+abstract class HeapByteBuffer extends ByteBuffer {

      protected final byte[] backingArray;

@@ -261,4 +261,54 @@ abstract class HeapByteBuffer extends Ba
              backingArray[baseOffset] = (byte) (value&  0xFF);
          }
      }
+
+    @Override
+    public final CharBuffer asCharBuffer() {
+        return CharToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final DoubleBuffer asDoubleBuffer() {
+        return DoubleToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final FloatBuffer asFloatBuffer() {
+        return FloatToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final IntBuffer asIntBuffer() {
+        return IntToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final LongBuffer asLongBuffer() {
+        return LongToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final ShortBuffer asShortBuffer() {
+        return ShortToByteBufferAdapter.wrap(this);
+    }
+
+    @Override
+    public final char getChar() {
+        return (char) getShort();
+    }
+
+    @Override
+    public final char getChar(int index) {
+        return (char) getShort(index);
+    }
+
+    @Override
+    public final ByteBuffer putChar(char value) {
+        return putShort((short) value);
+    }
+
+    @Override
+    public final ByteBuffer putChar(int index, char value) {
+        return putShort(index, (short) value);
+    }
  }

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/MappedByteBuffer.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/MappedByteBuffer.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/MappedByteBuffer.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/MappedByteBuffer.java
 Tue Jun  1 10:51:17 2010
@@ -37,34 +37,13 @@ import org.apache.harmony.nio.internal.D
   */
  public abstract class MappedByteBuffer extends ByteBuffer {

-    final DirectByteBuffer wrapped;
+    // This is the base address of the direct byte buffer memory.
+    protected PlatformAddress address;

-    private int mapMode;
+    protected int mapMode;

-    MappedByteBuffer(ByteBuffer directBuffer) {
-        super(directBuffer.capacity);
-        if (!directBuffer.isDirect()) {
-            throw new IllegalArgumentException();
-        }
-        this.wrapped = (DirectByteBuffer) directBuffer;
-
-    }
-
-    MappedByteBuffer(PlatformAddress addr, int capa, int offset, int mode) {
-        super(capa);
-        mapMode = mode;
-        switch (mapMode) {
-            case IMemorySystem.MMAP_READ_ONLY:
-                wrapped = new ReadOnlyDirectByteBuffer(addr, capa, offset);
-                break;
-            case IMemorySystem.MMAP_READ_WRITE:
-            case IMemorySystem.MMAP_WRITE_COPY:
-                wrapped = new ReadWriteDirectByteBuffer(addr, capa, offset);
-                break;
-            default:
-                throw new IllegalArgumentException();
-        }
-        addr.autoFree();
+    MappedByteBuffer(int capacity) {
+        super(capacity);
      }

      /**
@@ -76,8 +55,7 @@ public abstract class MappedByteBuffer e
       *         otherwise.
       */
      public final boolean isLoaded() {
-        return ((MappedPlatformAddress) ((DirectBuffer) wrapped)
-                .getBaseAddress()).mmapIsLoaded();
+        return ((MappedPlatformAddress)address).mmapIsLoaded();
      }

      /**
@@ -87,8 +65,7 @@ public abstract class MappedByteBuffer e
       * @return this buffer.
       */
      public final MappedByteBuffer load() {
-        ((MappedPlatformAddress) ((DirectBuffer) wrapped).getBaseAddress())
-                .mmapLoad();
+        ((MappedPlatformAddress)address).mmapLoad();
          return this;
      }

@@ -102,8 +79,7 @@ public abstract class MappedByteBuffer e
       */
      public final MappedByteBuffer force() {
          if (mapMode == IMemorySystem.MMAP_READ_WRITE) {
-            ((MappedPlatformAddress) ((DirectBuffer) wrapped).getBaseAddress())
-                    .mmapFlush();
+            ((MappedPlatformAddress)address).mmapFlush();
          }
          return this;
      }

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadOnlyDirectByteBuffer.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadOnlyDirectByteBuffer.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadOnlyDirectByteBuffer.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadOnlyDirectByteBuffer.java
 Tue Jun  1 10:51:17 2010
@@ -48,6 +48,15 @@ final class ReadOnlyDirectByteBuffer ext
          super(address, capacity, offset);
      }

+    /*
+     * This constructor is specifically for MappedByteBuffer construction
+     */
+    protected ReadOnlyDirectByteBuffer(PlatformAddress address, int capacity,
+            int offset, int mapMode) {
+        super(address, capacity, offset);
+        this.mapMode = mapMode;
+    }
+
      @Override
      public ByteBuffer asReadOnlyBuffer() {
          return copy(this, mark);

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadWriteDirectByteBuffer.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadWriteDirectByteBuffer.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadWriteDirectByteBuffer.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/java/nio/ReadWriteDirectByteBuffer.java
 Tue Jun  1 10:51:17 2010
@@ -52,6 +52,15 @@ final class ReadWriteDirectByteBuffer ex
          super(address, capacity, offset);
      }

+    /*
+     * This constructor is specifically for MappedByteBuffer construction
+     */
+    ReadWriteDirectByteBuffer(PlatformAddress address, int capacity,
+            int offset, int mapMode) {
+        super(address, capacity, offset);
+        this.mapMode = mapMode;
+    }
+
      @Override
      public ByteBuffer asReadOnlyBuffer() {
          return ReadOnlyDirectByteBuffer.copy(this, mark);

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/org/apache/harmony/nio/internal/MappedByteBufferFactory.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/org/apache/harmony/nio/internal/MappedByteBufferFactory.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/org/apache/harmony/nio/internal/MappedByteBufferFactory.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/main/java/common/org/apache/harmony/nio/internal/MappedByteBufferFactory.java
 Tue Jun  1 10:51:17 2010
@@ -24,20 +24,45 @@ import java.security.PrivilegedAction;

  import org.apache.harmony.luni.platform.PlatformAddress;

+import org.apache.harmony.luni.platform.IMemorySystem;
+import org.apache.harmony.luni.platform.MappedPlatformAddress;
+import org.apache.harmony.luni.platform.PlatformAddress;
+import org.apache.harmony.nio.internal.DirectBuffer;
+
  class MappedByteBufferFactory {

-    static final Constructor<?>  constructor;
+    static final Constructor<?>  readOnlyConstructor;
+    static final Constructor<?>  readWriteConstructor;

      static {
-        constructor = AccessController
+        readOnlyConstructor = AccessController
+                .doPrivileged(new PrivilegedAction<Constructor<?>>() {
+                    public Constructor<?>  run() {
+                        try {
+                            Class<?>  wrapperClazz = ClassLoader
+                                    .getSystemClassLoader().loadClass(
+                                            
"java.nio.ReadOnlyDirectByteBuffer"); //$NON-NLS-1$
+                            Constructor<?>  result = wrapperClazz
+                                    .getDeclaredConstructor(new Class[] {
+                                            PlatformAddress.class, int.class,
+                                            int.class, int.class });
+                            result.setAccessible(true);
+                            return result;
+                        } catch (Exception e) {
+                            throw new RuntimeException(e);
+                        }
+                    }
+                });
+
+        readWriteConstructor = AccessController
                  .doPrivileged(new PrivilegedAction<Constructor<?>>() {
                      public Constructor<?>  run() {
                          try {
                              Class<?>  wrapperClazz = ClassLoader
                                      .getSystemClassLoader().loadClass(
-                                            
"java.nio.MappedByteBufferAdapter"); //$NON-NLS-1$
+                                            
"java.nio.ReadWriteDirectByteBuffer"); //$NON-NLS-1$
                              Constructor<?>  result = wrapperClazz
-                                    .getConstructor(new Class[] {
+                                    .getDeclaredConstructor(new Class[] {
                                              PlatformAddress.class, int.class,
                                              int.class, int.class });
                              result.setAccessible(true);
@@ -55,8 +80,18 @@ class MappedByteBufferFactory {
           * Spec points out explicitly that the size of map should be no 
greater
           * than Integer.MAX_VALUE, so long to int cast is safe here.
           */
-        return (MappedByteBuffer) constructor.newInstance(new Object[] { addr,
+        switch (mapmode) {
+            case IMemorySystem.MMAP_READ_ONLY:
+                return (MappedByteBuffer) readOnlyConstructor.newInstance(new 
Object[] { addr,
+                                                                         
Integer.valueOf((int)size), Integer.valueOf(offset),
+                                                                         
Integer.valueOf(mapmode) });
+            case IMemorySystem.MMAP_READ_WRITE:
+            case IMemorySystem.MMAP_WRITE_COPY:
+                return (MappedByteBuffer) readWriteConstructor.newInstance(new 
Object[] { addr,
                                                                           
Integer.valueOf((int)size), Integer.valueOf(offset),
                                                                           
Integer.valueOf(mapmode) });
+            default:
+                throw new IllegalArgumentException();
+        }
      }
  }

Modified: 
harmony/enhanced/java/trunk/classlib/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
URL: 
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java?rev=950012&r1=950011&r2=950012&view=diff
==============================================================================
--- 
harmony/enhanced/java/trunk/classlib/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
 (original)
+++ 
harmony/enhanced/java/trunk/classlib/modules/nio/src/test/java/common/org/apache/harmony/nio/tests/java/nio/MappedByteBufferTest.java
 Tue Jun  1 10:51:17 2010
@@ -208,4 +208,24 @@ public class MappedByteBufferTest extend
          emptyFile = File.createTempFile("harmony", "test");  
//$NON-NLS-1$//$NON-NLS-2$
          emptyFile.deleteOnExit();
      }
+
+    public void test_position() throws IOException {
+        File tmp = File.createTempFile("hmy", "tmp");
+        tmp.deleteOnExit();
+        RandomAccessFile f = new RandomAccessFile(tmp, "rw");
+        FileChannel ch = f.getChannel();
+        MappedByteBuffer mbb = ch.map(MapMode.READ_WRITE, 0L, 100L);
+        ch.close();
+
+        mbb.putInt(1, 1);
+        mbb.position(50);
+        mbb.putInt(50);
+
+        mbb.flip();
+        mbb.get();
+        assertEquals(1, mbb.getInt());
+
+        mbb.position(50);
+        assertEquals(50, mbb.getInt());
+    }
  }



   

--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU