harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oliver Deakin <oliver.dea...@googlemail.com>
Subject 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/
Date Tue, 01 Jun 2010 11:12:27 GMT
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, odeakin@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


Mime
View raw message