Performance problems when reading and writing *BufferData

[gouessej]

Hi

In my humble opinion, there are several performance problems in the reading and writing of *BufferData classes.

At first, the NIO buffer is duplicated when saving a FloatBufferData into a file. Then, another NIO buffer (ByteBuffer) is created. It means that 3 NIO buffers with the same content are used when saving a FloatBufferData. Absolute get() operations should be used to copy the data from the FloatBuffer to the byte array with a smaller memory footprint and it would be faster despite the absolute individual get() operations because of the skip of useless data copies. You can still modify the position of the buffer before copying the data into the array, use the relative bulk get() method and resets the position. This problem has a dramatic consequence on JFPSM which creates huge levels (the biggest one contains 255 484 triangles).

Then, when a NIO buffer have some content resident partially on disk and partially in memory, it is slower but still preferable not to use a relative bulk get() because such a call would load the whole content in memory whereas it is exactly what we try to avoid by using this kind of NIO buffer. You should at least do several smaller relative bulk get() calls.

Finally, it is sometimes impossible to create a very big direct NIO buffer whereas it is possible to create an indirect NIO buffer of the same size. Currently, we must use direct buffers when writing to get direct buffers when reading. It would be fine to have a flag to override this behavior.

I will be able to provide a patch for the 2 first aspects in some days but I would highly appreciable that someone else implements the very last trivial one:

Code: Select all

final boolean direct = ALWAYS_USE_DIRECT_NIO_BUFFERS || readBoolean(content);

Maybe it would be better as a field in BinaryInputCapsule. Best regards.

[gouessej]

Hi

Maybe the last option should be implemented in the BinaryOutputCapsule. I'm still investigating.

[gouessej]

I suggest this change as a first step:

Code: Select all

protected void write(final FloatBuffer source) throws IOException {
        if (source == null) {
            write(NULL_OBJECT);
            return;
        }

        // duplicate buffer to allow modification of limit/position without changing original.
        //final FloatBuffer value = source.duplicate();

        // write length
        final int length = source.limit();
        write(length);

        // write boolean for directness
        write(source.isDirect());

        // create little endian store
        final ByteBuffer buf = ByteBuffer.allocate(length * 4).order(ByteOrder.LITTLE_ENDIAN);

        // place buffer into store.
        int position = source.position();
        source.rewind();
        buf.asFloatBuffer().put(source);
        buf.rewind();
        source.position(position);

        // Pull out store as array
        final byte[] array = new byte[buf.capacity()];
        buf.get(array);

        // write to stream
        _baos.write(array);
    }

It allows to avoid creating a duplicated NIO buffer at the beginning. If the NIO buffer has a backing array, some other operations can be simplified (just get this float array, use Float.floatToIntBits(), convert it into little endian and put it into the byte array).

When reading a FloatBufferData in BinaryInputCapsule, if the float buffer has to be direct and if the native order is little endian, the direct byte buffer can be directly used with asFloatBuffer(), there is no need to create another direct buffer.

Code: Select all

final FloatBuffer value;
final boolean putNeeded;
if (direct) {
      if (ByteOrder.nativeOrder()==ByteOrder.LITTLE_ENDIAN) {
             value = buf.asFloatBuffer();
             putNeeded = false;
      }
      else {
             value = BufferUtils.createFloatBuffer(length);
             putNeeded = true;
      }
}
else {
       value = BufferUtils.createFloatBufferOnHeap(length);
       putNeeded = true;
}
if (putNeeded) {
      value.put(buf.asFloatBuffer());
      value.rewind();
}


Edit.: The duplicate() call has a very low cost on heap (non direct) buffers, it's almost the same with the direct buffers.

[renanse]

duplicate() does not double memory requirements, or make a new buffer... it only makes a new wrapper surrounding the same backing data object, just allowing you to have different limit, mark and position.

Code: Select all

    /**
     * Creates a new float buffer that shares this buffer's content.
     *
     * <p> The content of the new buffer will be that of this buffer.  Changes
     * to this buffer's content will be visible in the new buffer, and vice
     * versa; the two buffers' position, limit, and mark values will be
     * independent.
     *
     * <p> The new buffer's capacity, limit, position, and mark values will be
     * identical to those of this buffer.  The new buffer will be direct if,
     * and only if, this buffer is direct, and it will be read-only if, and
     * only if, this buffer is read-only.  </p>
     *
     * @return  The new float buffer
     */

[gouessej]

duplicate() does not double memory requirements, or make a new buffer... it only makes a new wrapper surrounding the same backing data object, just allowing you to have different limit, mark and position.

That's what I meant in my last sentence:

Edit.: The duplicate() call has a very low cost on heap (non direct) buffers, it's almost the same with the direct buffers.

However, creating another direct NIO float buffer instead of simply using a view on a direct byte buffer with the same native order when reading a FloatBufferData really doubles memory requirement and it is particularly annoying with direct NIO buffers as you can lack of memory on the native heap before lacking of memory on the Java heap, etc...

I'm going to provide some patches.

[gouessej]

First patch for BinaryInputCapsule:

Code: Select all

### Eclipse Workspace Patch 1.0
#P ardor3d-core
Index: src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java
===================================================================
--- src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java   (revision 1869)
+++ src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java   (working copy)
@@ -1240,10 +1240,24 @@
         _index += length * 4;

         // Convert to float buffer.
-        final FloatBuffer value = direct ? BufferUtils.createFloatBuffer(length) : BufferUtils
-                .createFloatBufferOnHeap(length);
-        value.put(buf.asFloatBuffer());
-        value.rewind();
+        final FloatBuffer value;
+        final boolean contentCopyRequired;
+        if (direct) {
+            if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
+                value = buf.asFloatBuffer();
+                contentCopyRequired = false;
+            } else {
+                value = BufferUtils.createFloatBuffer(length);
+                contentCopyRequired = true;
+            }
+        } else {
+            value = BufferUtils.createFloatBufferOnHeap(length);
+            contentCopyRequired = true;
+        }
+        if (contentCopyRequired) {
+            value.put(buf.asFloatBuffer());
+            value.rewind();
+        }
         return value;
     }



[gouessej]

Full patch:

Code: Select all

### Eclipse Workspace Patch 1.0
#P ardor3d-core
Index: src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java
===================================================================
--- src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java   (revision 1869)
+++ src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java   (working copy)
@@ -1240,10 +1240,24 @@
         _index += length * 4;

         // Convert to float buffer.
-        final FloatBuffer value = direct ? BufferUtils.createFloatBuffer(length) : BufferUtils
-                .createFloatBufferOnHeap(length);
-        value.put(buf.asFloatBuffer());
-        value.rewind();
+        final FloatBuffer value;
+        final boolean contentCopyRequired;
+        if (direct) {
+            if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
+                value = buf.asFloatBuffer();
+                contentCopyRequired = false;
+            } else {
+                value = BufferUtils.createFloatBuffer(length);
+                contentCopyRequired = true;
+            }
+        } else {
+            value = BufferUtils.createFloatBufferOnHeap(length);
+            contentCopyRequired = true;
+        }
+        if (contentCopyRequired) {
+            value.put(buf.asFloatBuffer());
+            value.rewind();
+        }
         return value;
     }

Index: src/main/java/com/ardor3d/util/export/binary/BinaryOutputCapsule.java
===================================================================
--- src/main/java/com/ardor3d/util/export/binary/BinaryOutputCapsule.java   (revision 1869)
+++ src/main/java/com/ardor3d/util/export/binary/BinaryOutputCapsule.java   (working copy)
@@ -39,11 +39,18 @@
     protected byte[] _bytes;
     protected BinaryExporter _exporter;
     protected BinaryClassObject _cObj;
+    protected boolean _forceDirectNioBuffers;

     public BinaryOutputCapsule(final BinaryExporter exporter, final BinaryClassObject bco) {
+        this(exporter, bco, false);
+    }
+
+    public BinaryOutputCapsule(final BinaryExporter exporter, final BinaryClassObject bco,
+            final boolean forceDirectNioBuffers) {
         _baos = new ByteArrayOutputStream();
         _exporter = exporter;
         _cObj = bco;
+        _forceDirectNioBuffers = forceDirectNioBuffers;
     }

     public void write(final byte value, final String name, final byte defVal) throws IOException {
@@ -812,27 +819,41 @@
             return;
         }

-        // duplicate buffer to allow modification of limit/position without changing original.
-        final FloatBuffer value = source.duplicate();
-
         // write length
-        final int length = value.limit();
+        final int length = source.limit();
         write(length);

         // write boolean for directness
-        write(value.isDirect());
+        write(_forceDirectNioBuffers || source.isDirect());

-        // create little endian store
-        final ByteBuffer buf = ByteBuffer.allocate(length * 4).order(ByteOrder.LITTLE_ENDIAN);
+        final byte[] array = new byte[length * 4];
+        if (source.hasArray()) {
+            // get the backing array of the source buffer
+            final float[] backingArray = source.array();

-        // place buffer into store.
-        value.rewind();
-        buf.asFloatBuffer().put(value);
-        buf.rewind();
+            // create a tiny store only to perform the conversion into little endian
+            final ByteBuffer buf = ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN);

-        // Pull out store as array
-        final byte[] array = new byte[buf.capacity()];
-        buf.get(array);
+            for (int i = 0; i < backingArray.length; i++) {
+                buf.putFloat(backingArray[i]).rewind();
+                buf.get(array, i * 4, 4).rewind();
+            }
+
+        } else {
+            // duplicate buffer to allow modification of limit/position without changing original.
+            final FloatBuffer value = source.duplicate();
+
+            // create little endian store
+            final ByteBuffer buf = ByteBuffer.allocate(length * 4).order(ByteOrder.LITTLE_ENDIAN);
+
+            // place buffer into store.
+            value.rewind();
+            buf.asFloatBuffer().put(value);
+            buf.rewind();
+
+            // Pull out store as array
+            buf.get(array);
+        }

         // write to stream
         _baos.write(array);


[gouessej]

Hi

My patch works fine, the data are not corrupted, please find below a short comparison done by running JFPSM and measuring the consumption with JVisualVM:
- without my patch: 86.392 seconds, 182 MB
- with my patch: 69.378 seconds, 133 MB

As you can see, it can be faster and less expensive (memory) on large data sets.

Latest patch including all my suggestions and for all types (int, short, float, byte):

Code: Select all

### Eclipse Workspace Patch 1.0
#P ardor3d-core
Index: src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java
===================================================================
--- src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java   (revision 1869)
+++ src/main/java/com/ardor3d/util/export/binary/BinaryInputCapsule.java   (working copy)
@@ -1240,10 +1240,24 @@
         _index += length * 4;

         // Convert to float buffer.
-        final FloatBuffer value = direct ? BufferUtils.createFloatBuffer(length) : BufferUtils
-                .createFloatBufferOnHeap(length);
-        value.put(buf.asFloatBuffer());
-        value.rewind();
+        final FloatBuffer value;
+        final boolean contentCopyRequired;
+        if (direct) {
+            if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
+                value = buf.asFloatBuffer();
+                contentCopyRequired = false;
+            } else {
+                value = BufferUtils.createFloatBuffer(length);
+                contentCopyRequired = true;
+            }
+        } else {
+            value = BufferUtils.createFloatBufferOnHeap(length);
+            contentCopyRequired = true;
+        }
+        if (contentCopyRequired) {
+            value.put(buf.asFloatBuffer());
+            value.rewind();
+        }
         return value;
     }

@@ -1264,10 +1278,24 @@
         _index += length * 4;

         // Convert to int buffer.
-        final IntBuffer value = direct ? BufferUtils.createIntBuffer(length) : BufferUtils
-                .createIntBufferOnHeap(length);
-        value.put(buf.asIntBuffer());
-        value.rewind();
+        final IntBuffer value;
+        final boolean contentCopyRequired;
+        if (direct) {
+            if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
+                value = buf.asIntBuffer();
+                contentCopyRequired = false;
+            } else {
+                value = BufferUtils.createIntBuffer(length);
+                contentCopyRequired = true;
+            }
+        } else {
+            value = BufferUtils.createIntBufferOnHeap(length);
+            contentCopyRequired = true;
+        }
+        if (contentCopyRequired) {
+            value.put(buf.asIntBuffer());
+            value.rewind();
+        }
         return value;
     }

@@ -1288,10 +1316,24 @@
         _index += length * 2;

         // Convert to short buffer.
-        final ShortBuffer value = direct ? BufferUtils.createShortBuffer(length) : BufferUtils
-                .createShortBufferOnHeap(length);
-        value.put(buf.asShortBuffer());
-        value.rewind();
+        final ShortBuffer value;
+        final boolean contentCopyRequired;
+        if (direct) {
+            if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
+                value = buf.asShortBuffer();
+                contentCopyRequired = false;
+            } else {
+                value = BufferUtils.createShortBuffer(length);
+                contentCopyRequired = true;
+            }
+        } else {
+            value = BufferUtils.createShortBufferOnHeap(length);
+            contentCopyRequired = true;
+        }
+        if (contentCopyRequired) {
+            value.put(buf.asShortBuffer());
+            value.rewind();
+        }
         return value;
     }

@@ -1312,10 +1354,24 @@
         _index += length;

         // Convert to platform endian buffer.
-        final ByteBuffer value = direct ? BufferUtils.createByteBuffer(length) : BufferUtils
-                .createByteBufferOnHeap(length);
-        value.put(buf);
-        value.rewind();
+        final ByteBuffer value;
+        final boolean contentCopyRequired;
+        if (direct) {
+            if (ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN) {
+                value = buf;
+                contentCopyRequired = false;
+            } else {
+                value = BufferUtils.createByteBuffer(length);
+                contentCopyRequired = true;
+            }
+        } else {
+            value = BufferUtils.createByteBufferOnHeap(length);
+            contentCopyRequired = true;
+        }
+        if (contentCopyRequired) {
+            value.put(buf);
+            value.rewind();
+        }
         return value;
     }

Index: src/main/java/com/ardor3d/util/export/binary/BinaryOutputCapsule.java
===================================================================
--- src/main/java/com/ardor3d/util/export/binary/BinaryOutputCapsule.java   (revision 1869)
+++ src/main/java/com/ardor3d/util/export/binary/BinaryOutputCapsule.java   (working copy)
@@ -39,11 +39,18 @@
     protected byte[] _bytes;
     protected BinaryExporter _exporter;
     protected BinaryClassObject _cObj;
+    protected boolean _forceDirectNioBuffers;

     public BinaryOutputCapsule(final BinaryExporter exporter, final BinaryClassObject bco) {
+        this(exporter, bco, false);
+    }
+
+    public BinaryOutputCapsule(final BinaryExporter exporter, final BinaryClassObject bco,
+            final boolean forceDirectNioBuffers) {
         _baos = new ByteArrayOutputStream();
         _exporter = exporter;
         _cObj = bco;
+        _forceDirectNioBuffers = forceDirectNioBuffers;
     }

     public void write(final byte value, final String name, final byte defVal) throws IOException {
@@ -812,27 +819,42 @@
             return;
         }

-        // duplicate buffer to allow modification of limit/position without changing original.
-        final FloatBuffer value = source.duplicate();
-
+        final int sizeof = 4;
         // write length
-        final int length = value.limit();
+        final int length = source.limit();
         write(length);

         // write boolean for directness
-        write(value.isDirect());
+        write(_forceDirectNioBuffers || source.isDirect());
+
+        final byte[] array = new byte[length * sizeof];
+        if (source.hasArray()) {
+            // get the backing array of the source buffer
+            final float[] backingArray = source.array();
+
+            // create a tiny store only to perform the conversion into little endian
+            final ByteBuffer buf = ByteBuffer.allocate(sizeof).order(ByteOrder.LITTLE_ENDIAN);
+
+            for (int i = 0; i < backingArray.length; i++) {
+                buf.putFloat(backingArray[i]).rewind();
+                buf.get(array, i * sizeof, sizeof).rewind();
+            }

-        // create little endian store
-        final ByteBuffer buf = ByteBuffer.allocate(length * 4).order(ByteOrder.LITTLE_ENDIAN);
+        } else {
+            // duplicate buffer to allow modification of limit/position without changing original.
+            final FloatBuffer value = source.duplicate();

-        // place buffer into store.
-        value.rewind();
-        buf.asFloatBuffer().put(value);
-        buf.rewind();
+            // create little endian store
+            final ByteBuffer buf = ByteBuffer.allocate(length * sizeof).order(ByteOrder.LITTLE_ENDIAN);

-        // Pull out store as array
-        final byte[] array = new byte[buf.capacity()];
-        buf.get(array);
+            // place buffer into store.
+            value.rewind();
+            buf.asFloatBuffer().put(value);
+            buf.rewind();
+
+            // Pull out store as array
+            buf.get(array);
+        }

         // write to stream
         _baos.write(array);
@@ -845,27 +867,42 @@
             return;
         }

-        // duplicate buffer to allow modification of limit/position without changing original.
-        final IntBuffer value = source.duplicate();
-
+        final int sizeof = 4;
         // write length
-        final int length = value.limit();
+        final int length = source.limit();
         write(length);

         // write boolean for directness
-        write(value.isDirect());
+        write(_forceDirectNioBuffers || source.isDirect());
+
+        final byte[] array = new byte[length * sizeof];
+        if (source.hasArray()) {
+            // get the backing array of the source buffer
+            final int[] backingArray = source.array();
+
+            // create a tiny store only to perform the conversion into little endian
+            final ByteBuffer buf = ByteBuffer.allocate(sizeof).order(ByteOrder.LITTLE_ENDIAN);
+
+            for (int i = 0; i < backingArray.length; i++) {
+                buf.putInt(backingArray[i]).rewind();
+                buf.get(array, i * sizeof, sizeof).rewind();
+            }
+
+        } else {
+            // duplicate buffer to allow modification of limit/position without changing original.
+            final IntBuffer value = source.duplicate();

-        // create little endian store
-        final ByteBuffer buf = ByteBuffer.allocate(length * 4).order(ByteOrder.LITTLE_ENDIAN);
+            // create little endian store
+            final ByteBuffer buf = ByteBuffer.allocate(length * sizeof).order(ByteOrder.LITTLE_ENDIAN);

-        // place buffer into store. Rewind buffers
-        value.rewind();
-        buf.asIntBuffer().put(value);
-        buf.rewind();
+            // place buffer into store.
+            value.rewind();
+            buf.asIntBuffer().put(value);
+            buf.rewind();

-        // Pull out store as array
-        final byte[] array = new byte[buf.capacity()];
-        buf.get(array);
+            // Pull out store as array
+            buf.get(array);
+        }

         // write to stream
         _baos.write(array);
@@ -878,27 +915,42 @@
             return;
         }

-        // duplicate buffer to allow modification of limit/position without changing original.
-        final ShortBuffer value = source.duplicate();
-
+        final int sizeof = 2;
         // write length
-        final int length = value.limit();
+        final int length = source.limit();
         write(length);

         // write boolean for directness
-        write(value.isDirect());
+        write(_forceDirectNioBuffers || source.isDirect());
+
+        final byte[] array = new byte[length * sizeof];
+        if (source.hasArray()) {
+            // get the backing array of the source buffer
+            final short[] backingArray = source.array();
+
+            // create a tiny store only to perform the conversion into little endian
+            final ByteBuffer buf = ByteBuffer.allocate(sizeof).order(ByteOrder.LITTLE_ENDIAN);

-        // create little endian store
-        final ByteBuffer buf = ByteBuffer.allocate(length * 2).order(ByteOrder.LITTLE_ENDIAN);
+            for (int i = 0; i < backingArray.length; i++) {
+                buf.putShort(backingArray[i]).rewind();
+                buf.get(array, i * sizeof, sizeof).rewind();
+            }

-        // place buffer into store. Rewind buffers
-        value.rewind();
-        buf.asShortBuffer().put(value);
-        buf.rewind();
+        } else {
+            // duplicate buffer to allow modification of limit/position without changing original.
+            final ShortBuffer value = source.duplicate();

-        // Pull out store as array
-        final byte[] array = new byte[buf.capacity()];
-        buf.get(array);
+            // create little endian store
+            final ByteBuffer buf = ByteBuffer.allocate(length * sizeof).order(ByteOrder.LITTLE_ENDIAN);
+
+            // place buffer into store.
+            value.rewind();
+            buf.asShortBuffer().put(value);
+            buf.rewind();
+
+            // Pull out store as array
+            buf.get(array);
+        }

         // write to stream
         _baos.write(array);
@@ -911,20 +963,25 @@
             return;
         }

-        // duplicate buffer to allow modification of limit/position without changing original.
-        final ByteBuffer value = source.duplicate();
-
         // write length
-        final int length = value.limit();
+        final int length = source.limit();
         write(length);

         // write boolean for directness
-        write(value.isDirect());
+        write(_forceDirectNioBuffers || source.isDirect());

-        // Pull out value as array
-        final byte[] array = new byte[length];
-        value.rewind();
-        value.get(array);
+        final byte[] array;
+        if (source.hasArray()) {
+            array = source.array();
+        } else {
+            // duplicate buffer to allow modification of limit/position without changing original.
+            final ByteBuffer value = source.duplicate();
+
+            // Pull out value as array
+            array = new byte[length];
+            value.rewind();
+            value.get(array);
+        }

         // write to stream
         _baos.write(array);


[renanse]

Got these changes locally and will promote to trunk soon. (Was a pain because eclipse flat out refused to apply those patches. )

[gouessej]

Got these changes locally and will promote to trunk soon. (Was a pain because eclipse flat out refused to apply those patches. )

Thank you very much. I used the latest revision when creating my patch. I hope such a problem won't happen again.