Details

    • Type: New Feature New Feature
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 2.0.8
    • Component/s: Core
    • Labels:
      None

      Description

      Provide a way to create a large IoBuffer from several smaller IoBuffers, without copying the underlying data.

      It would probably be acceptable to constrain the composite buffer in various ways, for example by disallowing autoexpanding or otherwise changing the capacity, the implementation could be greatly simplified.

      The goal is to be able to process large messages with a minimum of copying.

      1. mina-composite-20080515.patch.gz
        9 kB
        Rich Dougherty
      2. mina-composite-20080517.patch
        64 kB
        Rich Dougherty
      3. mina-composite-20080521.patch
        35 kB
        Rich Dougherty
      4. mina-composite-20080521-2.patch
        41 kB
        Rich Dougherty
      5. mina-composite-20080723-1.patch
        4 kB
        Rich Dougherty
      6. mina-composite-20080723-2.patch
        2 kB
        Rich Dougherty

        Activity

        David M. Lloyd created issue -
        Hide
        Johannes Ulfkjær Jensen added a comment -

        In addition to the mentioned benefits, this could also serve as the way to introduce gathering write to the framework by adding IoBuffer.isComposite(), IoBuffer.getComposites(), in the same style of having backing arrays. That being unless I missed something, which could happen as I do not know the framework that well.

        This same class could also be used to implement scatter read, but as far as I can see, that would mean every instance of IoAcceptor (or SessionConfig?) would need a specific IoBufferAllocator.

        Show
        Johannes Ulfkjær Jensen added a comment - In addition to the mentioned benefits, this could also serve as the way to introduce gathering write to the framework by adding IoBuffer.isComposite(), IoBuffer.getComposites(), in the same style of having backing arrays. That being unless I missed something, which could happen as I do not know the framework that well. This same class could also be used to implement scatter read, but as far as I can see, that would mean every instance of IoAcceptor (or SessionConfig?) would need a specific IoBufferAllocator.
        Hide
        Trustin Lee added a comment -

        Thanks Johannes for the comment. It is very helpful. Please keep posting any new ideas so we can roll out something cool.

        Show
        Trustin Lee added a comment - Thanks Johannes for the comment. It is very helpful. Please keep posting any new ideas so we can roll out something cool.
        Hide
        Rich Dougherty added a comment -

        Attaching file from this message (http://www.nabble.com/Re%3A-Streams-and-disposal-of-ByteBuffers--Was%3A-Re%3A-Redesigning-IoBuffer...--p17251948.html), to make license approval explicit.

        Note that ByteArrayList contains code I originally wrote for the Commons Collections project, although I lost the previous license headers somewhere along the way.

        Excerpt from email:

        Since my last email I've spent a bit of time playing around with a set of classes and interfaces that (hopefully) provide the sort of functionality that we talk about. They're not quite ready, but I think it might be worth sharing them, so you can consider them for your new branch.

        I've attached the source for these files. They're not complete yet or very well tested (e.g. flushing doesn't work properly), but I think I'm reasonably happy with the design. Notably there's no mark/reset functionality, nor is there support for reading/writing a wide variety of types (just byte/ByteBuffer/int). These features should be easy to add. I just wanted to keep it as simple as possible initially.

        The key thing I've done is factored the design into multiple classes and interfaces. This move away from a single, monolithic class is intentional. Having multiple classes allows users to pick the implementation that suits a particular usage. It also dramatically simplifies implementation since we can use lower-level classes to build our higher-level ones, rather than trying to do everything within a single class. e.g. Stream classes are much easier to write once a good composite buffer class has been written.

        Here's a summary of the classes: (Names are just placeholders.)

        • BufferByteArray: A class that wraps ByteBuffer, providing simple utility methods and especially a free method to support pooling.
        • CompositeByteArray: There is a class with the same interface that supports multiple buffers with O(1) adding and removing.
        • *ByteArray.Cursor: Stores position information for a ByteArray. Keeping this information separate makes the classes simpler, and gives users more flexibility (e.g. reading and writing at separate positions at the same time).
        • CompositeByteArrayRelativeReader/Writer: Restrictive, relative-access-only stream interfaces, backed by a CompositeByteArray. The benefit of these stream interfaces is that they control access to the underlying buffers, and so can do certain things automatically for the user (e.g. freeing buffers).

        Anyway the design should allow all the big features we've been talking about:

        • zero-copy reads
        • gathering writes
        • optional asynchronous stream interface with auto-freeing/auto-allocating/auto-flushing of ByteBuffers
        Show
        Rich Dougherty added a comment - Attaching file from this message ( http://www.nabble.com/Re%3A-Streams-and-disposal-of-ByteBuffers--Was%3A-Re%3A-Redesigning-IoBuffer...--p17251948.html ), to make license approval explicit. Note that ByteArrayList contains code I originally wrote for the Commons Collections project, although I lost the previous license headers somewhere along the way. Excerpt from email: Since my last email I've spent a bit of time playing around with a set of classes and interfaces that (hopefully) provide the sort of functionality that we talk about. They're not quite ready, but I think it might be worth sharing them, so you can consider them for your new branch. I've attached the source for these files. They're not complete yet or very well tested (e.g. flushing doesn't work properly), but I think I'm reasonably happy with the design. Notably there's no mark/reset functionality, nor is there support for reading/writing a wide variety of types (just byte/ByteBuffer/int). These features should be easy to add. I just wanted to keep it as simple as possible initially. The key thing I've done is factored the design into multiple classes and interfaces. This move away from a single, monolithic class is intentional. Having multiple classes allows users to pick the implementation that suits a particular usage. It also dramatically simplifies implementation since we can use lower-level classes to build our higher-level ones, rather than trying to do everything within a single class. e.g. Stream classes are much easier to write once a good composite buffer class has been written. Here's a summary of the classes: (Names are just placeholders.) BufferByteArray: A class that wraps ByteBuffer, providing simple utility methods and especially a free method to support pooling. CompositeByteArray: There is a class with the same interface that supports multiple buffers with O(1) adding and removing. *ByteArray.Cursor: Stores position information for a ByteArray. Keeping this information separate makes the classes simpler, and gives users more flexibility (e.g. reading and writing at separate positions at the same time). CompositeByteArrayRelativeReader/Writer: Restrictive, relative-access-only stream interfaces, backed by a CompositeByteArray. The benefit of these stream interfaces is that they control access to the underlying buffers, and so can do certain things automatically for the user (e.g. freeing buffers). Anyway the design should allow all the big features we've been talking about: zero-copy reads gathering writes optional asynchronous stream interface with auto-freeing/auto-allocating/auto-flushing of ByteBuffers
        Rich Dougherty made changes -
        Field Original Value New Value
        Attachment mina-composite-20080515.patch.gz [ 12382144 ]
        Hide
        Rich Dougherty added a comment -

        Updated patch:

        • improved tests and fixed bugs
        • got autoFlush working (at least for a simple test)
        • add putInt() method
        • change start()/length() to first()/last(), to make meaning clearer
        • altered CursorListener interface
        Show
        Rich Dougherty added a comment - Updated patch: improved tests and fixed bugs got autoFlush working (at least for a simple test) add putInt() method change start()/length() to first()/last(), to make meaning clearer altered CursorListener interface
        Rich Dougherty made changes -
        Attachment mina-composite-20080517.patch [ 12382239 ]
        Rich Dougherty made changes -
        Attachment mina-composite-20080517.patch [ 12382239 ]
        Hide
        Rich Dougherty added a comment -

        (Updated patch - previous version was patch against already-patched code, not against trunk.)

        Show
        Rich Dougherty added a comment - (Updated patch - previous version was patch against already-patched code, not against trunk.)
        Rich Dougherty made changes -
        Attachment mina-composite-20080517.patch [ 12382254 ]
        Hide
        Rich Dougherty added a comment -

        This patch is against the code in the 'buffer' branch.

        • Added support for all primitive types (and wrote a basic test).
        • Added ByteArray.equals() method (not yet tested).
        Show
        Rich Dougherty added a comment - This patch is against the code in the 'buffer' branch. Added support for all primitive types (and wrote a basic test). Added ByteArray.equals() method (not yet tested).
        Rich Dougherty made changes -
        Attachment mina-composite-20080521.patch [ 12382431 ]
        Hide
        Rich Dougherty added a comment -

        This patch is against the code in the 'buffer' branch.

        • Added slice() operations.
        • Added skip() operations to relative readers and cursors.

        (Sorry about patch spam! I'm just aware that this area is being actively worked on right now, so I'm trying to provide any useful functionality as I develop it.)

        Show
        Rich Dougherty added a comment - This patch is against the code in the 'buffer' branch. Added slice() operations. Added skip() operations to relative readers and cursors. (Sorry about patch spam! I'm just aware that this area is being actively worked on right now, so I'm trying to provide any useful functionality as I develop it.)
        Rich Dougherty made changes -
        Attachment mina-composite-20080521-2.patch [ 12382447 ]
        Julien Vermillard made changes -
        Fix Version/s 2.0.0-M3 [ 12313299 ]
        Fix Version/s 2.0.0-M2 [ 12312861 ]
        Hide
        Mark Webb added a comment -

        Rich,

        Thanks for the work that you have done on this. Could you please tell me if the "mina-composite-20080521-2.patch" is the latest version of the patch. There have been many MINA source code structure changes and want to try integrating your code.

        Thanks
        Mark

        Show
        Mark Webb added a comment - Rich, Thanks for the work that you have done on this. Could you please tell me if the "mina-composite-20080521-2.patch" is the latest version of the patch. There have been many MINA source code structure changes and want to try integrating your code. Thanks Mark
        Hide
        Rich Dougherty added a comment -

        Hi Mark

        Sounds good. Yes, that patch is the latest. It was applied against the 'buffer' branch.

        It might be easiest just to copy the code from the 'buffer' branch, rather than reconstructing from the series of patches listed here. The entire contribution is contained in a single package - org.apache.mina.util.byteaccess.

        http://svn.apache.org/repos/asf/mina/branches/buffer/core/src/main/java/org/apache/mina/util/byteaccess/

        There's also a test, which should probably be moved to the same package.

        http://svn.apache.org/repos/asf/mina/branches/buffer/core/src/test/java/org/apache/mina/common/ByteAccessTest.java

        Cheers
        Rich

        Show
        Rich Dougherty added a comment - Hi Mark Sounds good. Yes, that patch is the latest. It was applied against the 'buffer' branch. It might be easiest just to copy the code from the 'buffer' branch, rather than reconstructing from the series of patches listed here. The entire contribution is contained in a single package - org.apache.mina.util.byteaccess. http://svn.apache.org/repos/asf/mina/branches/buffer/core/src/main/java/org/apache/mina/util/byteaccess/ There's also a test, which should probably be moved to the same package. http://svn.apache.org/repos/asf/mina/branches/buffer/core/src/test/java/org/apache/mina/common/ByteAccessTest.java Cheers Rich
        Hide
        Mark Webb added a comment -

        Rich,

        I have integrated the code into the version 2.0 baseline (trunk). I am working on some more tests and and trying to figure out the optimal way to create a simple CompositeByteArray containing a bunch of IoBuffers that contain strings. Here is some pseudo-code:

        CompositeByteArray cba = new CompositeByteArray();
        IoBuffer one = IoBuffer.wrap("Hello");
        cba.put( one );
        IoBuffer two = IoBuffer.wrap("Mina");
        cba.put( two );
        IoBuffer three = IoBuffer.wrap("World");
        cba.put( three );

        System.out.println( cba );

        I would like to see the contents of all 3 IoBuffers printed out together. Of course this simple example would be expanded to include more testing, but I have tried a couple different scenarios on putting multiple IoBuffers into an instance of the CompositeByteArray and have been unsuccessful in getting a single buffer back out.

        Is maybe CompositeByteArray not the correct class?

        Thanks

        Show
        Mark Webb added a comment - Rich, I have integrated the code into the version 2.0 baseline (trunk). I am working on some more tests and and trying to figure out the optimal way to create a simple CompositeByteArray containing a bunch of IoBuffers that contain strings. Here is some pseudo-code: CompositeByteArray cba = new CompositeByteArray(); IoBuffer one = IoBuffer.wrap("Hello"); cba.put( one ); IoBuffer two = IoBuffer.wrap("Mina"); cba.put( two ); IoBuffer three = IoBuffer.wrap("World"); cba.put( three ); System.out.println( cba ); I would like to see the contents of all 3 IoBuffers printed out together. Of course this simple example would be expanded to include more testing, but I have tried a couple different scenarios on putting multiple IoBuffers into an instance of the CompositeByteArray and have been unsuccessful in getting a single buffer back out. Is maybe CompositeByteArray not the correct class? Thanks
        Hide
        Rich Dougherty added a comment - - edited

        Patch against 'buffer' branch which fixes issues using get/put on larger ByteBuffers. (mina-composite-20080723-1.patch)

        Show
        Rich Dougherty added a comment - - edited Patch against 'buffer' branch which fixes issues using get/put on larger ByteBuffers. (mina-composite-20080723-1.patch)
        Rich Dougherty made changes -
        Attachment mina-composite-20080723-1.patch [ 12386699 ]
        Hide
        Rich Dougherty added a comment - - edited

        Patch against 'buffer' branch with example of concatenating Strings with a CompositeByteArray. (mina-composite-20080723-2.patch)

        Show
        Rich Dougherty added a comment - - edited Patch against 'buffer' branch with example of concatenating Strings with a CompositeByteArray. (mina-composite-20080723-2.patch)
        Rich Dougherty made changes -
        Attachment mina-composite-20080723-2.patch [ 12386700 ]
        Hide
        Rich Dougherty added a comment -

        Hi Mark

        I've added a patch which shows how this can be done, but here's a bit more explanation. For reference, here's the core code:

        + ByteArray ba1 = wrapString("Hello");
        + ByteArray ba2 = wrapString("MINA");
        + ByteArray ba3 = wrapString("World");
        +
        + CompositeByteArray cba = new CompositeByteArray();
        + cba.addLast(ba1);
        + cba.addLast(ba2);
        + cba.addLast(ba3);
        +
        + assertEquals("HelloMINAWorld", toString(cba));

        CompositeByteArray is the right class for that task, and the method you want is 'addLast'. You can also 'addFirst' if you like. If you want transparent automatic expansion of the CompositeByteArray, using only 'put' methods, check out CompositeByteArrayRelativeWriter.

        A CompositeByteArray is made of other ByteArrays. This means you can't add IoBuffers directly; you'd need to convert or copy them into a ByteArray first. In your case you can just make the Strings directly into ByteArrays - see the 'wrapString' method in the patch.

        Currently ByteArrays are quite low-level. They only really deal with getting and putting ByteBuffers and primitives. So there's currently no support for getting or putting Strings, IoBuffers and byte[]s. However, I think these things should be fairly easy to add, if they're needed.

        Cheers
        Rich

        Show
        Rich Dougherty added a comment - Hi Mark I've added a patch which shows how this can be done, but here's a bit more explanation. For reference, here's the core code: + ByteArray ba1 = wrapString("Hello"); + ByteArray ba2 = wrapString("MINA"); + ByteArray ba3 = wrapString("World"); + + CompositeByteArray cba = new CompositeByteArray(); + cba.addLast(ba1); + cba.addLast(ba2); + cba.addLast(ba3); + + assertEquals("HelloMINAWorld", toString(cba)); CompositeByteArray is the right class for that task, and the method you want is 'addLast'. You can also 'addFirst' if you like. If you want transparent automatic expansion of the CompositeByteArray, using only 'put' methods, check out CompositeByteArrayRelativeWriter. A CompositeByteArray is made of other ByteArrays. This means you can't add IoBuffers directly; you'd need to convert or copy them into a ByteArray first. In your case you can just make the Strings directly into ByteArrays - see the 'wrapString' method in the patch. Currently ByteArrays are quite low-level. They only really deal with getting and putting ByteBuffers and primitives. So there's currently no support for getting or putting Strings, IoBuffers and byte[]s. However, I think these things should be fairly easy to add, if they're needed. Cheers Rich
        Mark Webb made changes -
        Assignee Mark Webb [ elihusmails ]
        Julien Vermillard made changes -
        Fix Version/s 2.0.0-M3 [ 12313299 ]
        Fix Version/s 2.0.0-M4 [ 12313348 ]
        Hide
        Julien Vermillard added a comment -

        today I was digging in old asyncweb codebase because at the time it was using some streams & byte arrays as HttpMessage contents and not IoBuffer. Here a link to the implementation : https://svn.safehaus.org/repos/asyncweb/branches/asyncweb-codefreeze-02132006/src/org/safehaus/asyncweb/io/ (mainly Bytes.java and BytesOutputStream.java). Perhaps it can give some inspiration

        Show
        Julien Vermillard added a comment - today I was digging in old asyncweb codebase because at the time it was using some streams & byte arrays as HttpMessage contents and not IoBuffer. Here a link to the implementation : https://svn.safehaus.org/repos/asyncweb/branches/asyncweb-codefreeze-02132006/src/org/safehaus/asyncweb/io/ (mainly Bytes.java and BytesOutputStream.java). Perhaps it can give some inspiration
        Hide
        Mark Webb added a comment -

        I have checked in the code. Please take a look and let me know what you think.

        Many thanks to Rich as I think he did a fantastic job with the code. I only ran some sanity checks, formatted the code, javadoc'd the code and made sure it works with the latest trunk,

        Show
        Mark Webb added a comment - I have checked in the code. Please take a look and let me know what you think. Many thanks to Rich as I think he did a fantastic job with the code. I only ran some sanity checks, formatted the code, javadoc'd the code and made sure it works with the latest trunk,
        Hide
        Emmanuel Lecharny added a comment -

        This is a major improvement, but it will be done in 3.0

        Show
        Emmanuel Lecharny added a comment - This is a major improvement, but it will be done in 3.0
        Emmanuel Lecharny made changes -
        Fix Version/s 3.0.0-M1 [ 12313531 ]
        Fix Version/s 2.0.0-M4 [ 12313348 ]
        Hide
        Mark Webb added a comment -

        The code submitted by David Lloyd has been placed in the baseline. Unless there are problems, this should be closed.

        Show
        Mark Webb added a comment - The code submitted by David Lloyd has been placed in the baseline. Unless there are problems, this should be closed.
        Hide
        Emmanuel Lecharny added a comment -

        Yes, I know it's in the base line, but it's not used atm. So I prefer to keep the issue open, until we use this code in the chain (hich is not the case atm).

        This is the reason why I postponed the issue to 3.0.

        Is that OK ?

        Show
        Emmanuel Lecharny added a comment - Yes, I know it's in the base line, but it's not used atm. So I prefer to keep the issue open, until we use this code in the chain (hich is not the case atm). This is the reason why I postponed the issue to 3.0. Is that OK ?
        Hide
        Mark Webb added a comment -

        Fine by me.

        Show
        Mark Webb added a comment - Fine by me.
        Hide
        Edouard De Oliveira added a comment -

        Should the source code be kept in 2.0 codebase if the issue is postponed ?

        Show
        Edouard De Oliveira added a comment - Should the source code be kept in 2.0 codebase if the issue is postponed ?
        Hide
        Emmanuel Lecharny added a comment -

        We will move it to a mina-3.0 branch soon. We can clean this part of the code during the 2.0-RC1 documentation phase.

        Show
        Emmanuel Lecharny added a comment - We will move it to a mina-3.0 branch soon. We can clean this part of the code during the 2.0-RC1 documentation phase.
        Julien Vermillard made changes -
        Fix Version/s 2.0.6 [ 12316652 ]
        Fix Version/s 3.0.0-M1 [ 12313531 ]
        Emmanuel Lecharny made changes -
        Fix Version/s 2.0.8 [ 12323342 ]
        Fix Version/s 2.0.6 [ 12316652 ]

          People

          • Assignee:
            Mark Webb
            Reporter:
            David M. Lloyd
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development