Re: FYI: Patch: java.nio: Reimplementing byte buffer getType/putTypemethods

[Eric Blake <ebb9 at byu dot net>]

I'm not sure this is completely correct.

Michael Koch wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi list,
>
>
> I commited the attached patch to make ByteBuffer implementations work 
> and easier to maintain. There is still some room for optimization but 
> having a working version is more important for now. ;-)
>
> +  
> +  public static final ByteBuffer putChar (ByteBuffer buffer, char value)
> +  {
> +    checkRemainingForWrite (buffer, 2);
> +
> +    if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
> +      {
> +        buffer.put ((byte) (((int) value) & 0x00ff));
                                ^^^^^
Overkill - value is converted to int automatically by the & operator. (Applies 
in many places throughout the file).

> +  public static final ByteBuffer putChar (ByteBuffer buffer, int index,
> +                                          char value)
> +  {
> +    checkAvailableForWrite (buffer, index, 2);
> +
> +    if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
> +      {
> +        buffer.put (index + 1, (byte) ((((int) value) & 0x00ff) >> 8));
> +        buffer.put (index, (byte) (((int) value) & 0xff00));

Question - does the write order have any bearing on efficiency?  Writing 
'index' before 'index + 1' may be more efficient on the underlying stream if 
it prevents a backwards seek.  Or I may just be spouting hot air here. :)

> +  public static final long getLong (ByteBuffer buffer)
> +  {
> +    checkRemainingForRead (buffer, 8);
> +
> +    if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
> +      {
> +        return (long) (((buffer.get() & 0xff))
> +                       + ((buffer.get() & 0xff) << 8)
> +                       + ((buffer.get() & 0xff) << 16)
> +                       + ((buffer.get() & 0xff) << 24)
> +                       + ((buffer.get() & 0xff) << 32)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is still being done on an int, so it is the same as << 0 (not what you 
intended).  You need to promote the internal expressions to long before the shift.
> +                       + ((buffer.get() & 0xff) << 40)
> +                       + ((buffer.get() & 0xff) << 48)
> +                       + ((buffer.get() & 0xff) << 56));

Something like:
    return ((buffer.get() & 0xff)
            + ((buffer.get() & 0xff) << 8)
            + ((buffer.get() & 0xff) << 16)
            + ((buffer.get() & 0xff) << 24)
            + ((buffer.get() & 0xffL) << 32)
            + ((buffer.get() & 0xffL) << 40)
            + ((buffer.get() & 0xffL) << 48)
            + ((buffer.get() & 0xffL) << 56));

> +  public static final ByteBuffer putLong (ByteBuffer buffer, long value)
> +  {
> +    checkRemainingForWrite (buffer, 8);
> +
> +    if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
> +      {
> +        buffer.put ((byte) (value & 0xff00000000000000L));
                                       ^^^^^^^^^^^^^^^^^^^
Wrong constants.  You are unintentionally writing 8 consecutive \0 bytes here.

Also, when compiling to a .class file, you will be more efficient by replacing 
'(byte) ((value & 0xff00000000000000L) >> 56)' with
'(byte) (((int) (value >> 56)) & 0xff)' - you are repeating fewer constants in 
the constant pool, and the bytecode favors loading an int over loading a long. 
  Even .o files might benefit on 32-bit processors by leaving 64-bit ops as 
soon as possible, depending on the optimizer's abilities.

> +        buffer.put ((byte) ((value & 0x00ff000000000000L) >> 8));
> +        buffer.put ((byte) ((value & 0x0000ff0000000000L) >> 16));
> +        buffer.put ((byte) ((value & 0x000000ff00000000L) >> 24));
> +        buffer.put ((byte) ((value & 0x00000000ff000000L) >> 32));
> +        buffer.put ((byte) ((value & 0x0000000000ff0000L) >> 40));
> +        buffer.put ((byte) ((value & 0x000000000000ff00L) >> 48));
> +        buffer.put ((byte) ((value & 0x00000000000000ffL) >> 56));
> +      }
> +    else
       ^^^^
Minor nit: You are inconsistent in using else clauses or just falling through 
when doing 'if (buffer.order() == ByteOrder.LITTLE_ENDIAN)'.

> +  public static final long getLong (ByteBuffer buffer, int index)
> +  {
> +    checkAvailableForRead (buffer, index, 8);
> +
> +    if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
> +      {
> +        return (long) ((buffer.get (index) & 0xff)
> +                       + ((buffer.get (index + 1) & 0xff) << 8)
> +                       + ((buffer.get (index + 2) & 0xff) << 16)
> +                       + ((buffer.get (index + 3) & 0xff) << 24)
> +                       + ((buffer.get (index + 4) & 0xff) << 32)

Again, coerce to long sooner.

> +  public static final float getFloat (ByteBuffer buffer)
> +  {
> +    checkRemainingForRead (buffer, 4);
> +
> +    if (buffer.order() == ByteOrder.LITTLE_ENDIAN)
> +      {
> +        return (float) ((buffer.get() & 0xff)
> +                        + ((buffer.get() & 0xff) << 8)
> +                        + ((buffer.get() & 0xff) << 16)
> +                        + ((buffer.get() & 0xff) << 24));
Not what you meant.  Use Float.intBitsToFloat(), not a cast.  Same with 
Double.longBitsToDouble() on the get direction.  And use 
Float.floatToIntBits()/Double.doubleToLongBits() on the put direction.

--
Someday, I might put a cute statement here.

Eric Blake             ebb9@byu.net