Thrift
  1. Thrift
  2. THRIFT-1058

All ByteBuffers the client uses should be slice()ed

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.6
    • Fix Version/s: None
    • Component/s: Java - Library
    • Labels:
      None

      Description

      Right now the code does something like yay so:

      return ByteBuffer.wrap(array, pos, len);

      The only problem is that .wrap(byte[],int,int) returns a ByteBuffer with position equal to pos and limit set to pos+len. Which means users cant used 'rewind' or position(0).

      It would be cleaner if the contract was "ByteBuffers will have position=0, limit=how much data you have".

      The way to accomplish this would be to:

      return ByteBuffer.wrap(array, pos, len).slice();

      In exchange for 1 extra object alloc (which the GC can clean up quick) we get the benefit of a easier to use BB interface.

        Activity

        Hide
        Bryan Duxbury added a comment -

        What's the objection to slice()ing the buffers at use time? If it's already positioned at zero, then it won't cause any problems.

        @Jeff - your own numbers show 20% overhead, which, while not the 100% I reported earlier, is nothing to ignore. (We've worked hard to get less than 20% in the past.)

        Show
        Bryan Duxbury added a comment - What's the objection to slice()ing the buffers at use time? If it's already positioned at zero, then it won't cause any problems. @Jeff - your own numbers show 20% overhead, which, while not the 100% I reported earlier, is nothing to ignore. (We've worked hard to get less than 20% in the past.)
        Hide
        Nathaniel Cook added a comment -

        I stumbled across this because of the TFramedTransport. I wasn't using it for any of my services but then to support an HsHa server I started to wrap my normal socket transport in a framed one. Then it took a while but eventually I discovered that because of the framed transport the ByteBuffers I was getting had specific position and limit values set. So performing operations on the ByteBuffers caused several problems. Mostly that the buffers weren't reusable because I wasn't always reseting the position and limit. This inconsistent behavior could easily be solved by slicing the ByteBuffers and would make dealing them much easier.

        Show
        Nathaniel Cook added a comment - I stumbled across this because of the TFramedTransport. I wasn't using it for any of my services but then to support an HsHa server I started to wrap my normal socket transport in a framed one. Then it took a while but eventually I discovered that because of the framed transport the ByteBuffers I was getting had specific position and limit values set. So performing operations on the ByteBuffers caused several problems. Mostly that the buffers weren't reusable because I wasn't always reseting the position and limit. This inconsistent behavior could easily be solved by slicing the ByteBuffers and would make dealing them much easier.
        Hide
        Jeff Whiting added a comment -

        We have been running into this problem so I wanted to verify benchmark because it would be better to have the sliced buffer.

        My results don't seem to jive:

        Iterations: 100000000
        Wrap Time: 246ms
        Slice Time: 275ms
        Wrap & Slice Time: 309ms

        javac 1.6.0_24
        Java(TM) SE Runtime Environment (build 1.6.0_24-b07-334-10M3326)
        Java HotSpot(TM) 64-Bit Server VM (build 19.1-b02-334, mixed mode)

        So according to my benchmark there isn't much overhead to do the slice rather than the wrap. Making the change to slice would make it so the ByteBuffer ONLY gets access to its data which provides good encapsulation. Additionally it would improve the client code and allows the ByteBuffer to be reused.

        I'm including the benchmark source code so that way if I'm overlooking something you can let me know.

        Show
        Jeff Whiting added a comment - We have been running into this problem so I wanted to verify benchmark because it would be better to have the sliced buffer. My results don't seem to jive: Iterations: 100000000 Wrap Time: 246ms Slice Time: 275ms Wrap & Slice Time: 309ms javac 1.6.0_24 Java(TM) SE Runtime Environment (build 1.6.0_24-b07-334-10M3326) Java HotSpot(TM) 64-Bit Server VM (build 19.1-b02-334, mixed mode) So according to my benchmark there isn't much overhead to do the slice rather than the wrap. Making the change to slice would make it so the ByteBuffer ONLY gets access to its data which provides good encapsulation. Additionally it would improve the client code and allows the ByteBuffer to be reused. I'm including the benchmark source code so that way if I'm overlooking something you can let me know.
        Hide
        Bryan Duxbury added a comment -

        I did a micro benchmark and found that calling slice() takes pretty consistently double the time as just doing wrap(). I don't think the usability benefit (which is, to me, still somewhat dubious) is worth any performance hit.

        Show
        Bryan Duxbury added a comment - I did a micro benchmark and found that calling slice() takes pretty consistently double the time as just doing wrap(). I don't think the usability benefit (which is, to me, still somewhat dubious) is worth any performance hit.

          People

          • Assignee:
            Unassigned
            Reporter:
            ryan rawson
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development