MyFaces Trinidad
  1. MyFaces Trinidad
  2. TRINIDAD-2046

Copying data from inputStream to OuputStream needs appropriate buffer size

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2.14-core , 2.0.0-beta-2
    • Fix Version/s: None
    • Component/s: Components
    • Labels:

      Description

      In the files

      ./trinidad-1.2.14/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/image/cache/FileSystemImageCache.java line:955
      ./trinidad-1.2.14/trinidad-impl/src/main/java/org/apache/myfaces/trinidadinternal/image/painter/ImageUtils.java line:303

      The buffer size is fixed as 1024 bytes. With the size of the data varies, the performance can be damaged a lot.

      We need an appropriate buffer size which depends on the size of the data to be copied.

      This is the same as the Appache Bug (https://issues.apache.org/bugzilla/show_bug.cgi?id=32546)

        Activity

        Hide
        Scott O'Bryan added a comment - - edited

        Changing this to be an improvement since it doesn't actually cause a regression, simply a performance issue.

        Show
        Scott O'Bryan added a comment - - edited Changing this to be an improvement since it doesn't actually cause a regression, simply a performance issue.
        Hide
        Scott O'Bryan added a comment - - edited

        I read the parent bug and took a look at the patch. You're basically saying that the buffer should be the size of the content with a MAXIMUM_SIZE equal to 1024 bytes?

        Do you have a patch that you tried to show this fix had a significant improvement over the existing implementation?

        Show
        Scott O'Bryan added a comment - - edited I read the parent bug and took a look at the patch. You're basically saying that the buffer should be the size of the content with a MAXIMUM_SIZE equal to 1024 bytes? Do you have a patch that you tried to show this fix had a significant improvement over the existing implementation?
        Hide
        Xiaoming Shi added a comment - - edited

        Hi Scott,

        The buffer size should increase as the size of content increases, Still a MAXIMUM_SIZE needed. We don't want it to create too large a buffer.

        BTW, we have done unit test, and find that,
        to copy 1MB data,
        it takes 7282622 nano seconds with a buffer size 4096
        while it takes 11470883 nano seconds with a buffer size 1024

        Show
        Xiaoming Shi added a comment - - edited Hi Scott, The buffer size should increase as the size of content increases, Still a MAXIMUM_SIZE needed. We don't want it to create too large a buffer. BTW, we have done unit test, and find that, to copy 1MB data, it takes 7282622 nano seconds with a buffer size 4096 while it takes 11470883 nano seconds with a buffer size 1024
        Hide
        Scott O'Bryan added a comment - - edited

        Very cool. Thanks for testing that.. Good improvement.

        So I guess my next question is, does this have more to do with the max size of the buffer? For instance, if I made a hard-coded 4K Buffer, would that be just as fast as a creating one of arbitrary size UP TO 4K. The reason I ask is that it seems to me that you would want the max buffer size to be configurable and that the purposes of allocating smaller buffers was to preserve memory for smaller resources. Does this sound about right?

        Show
        Scott O'Bryan added a comment - - edited Very cool. Thanks for testing that.. Good improvement. So I guess my next question is, does this have more to do with the max size of the buffer? For instance, if I made a hard-coded 4K Buffer, would that be just as fast as a creating one of arbitrary size UP TO 4K. The reason I ask is that it seems to me that you would want the max buffer size to be configurable and that the purposes of allocating smaller buffers was to preserve memory for smaller resources. Does this sound about right?
        Hide
        Xiaoming Shi added a comment -

        Yes

        For resources that are smaller than the MAX_BUFFER size, we can just allocate a buffer with the size of the resource.

        Here is the final patch of the parent bug: https://issues.apache.org/bugzilla/attachment.cgi?id=13689

        Show
        Xiaoming Shi added a comment - Yes For resources that are smaller than the MAX_BUFFER size, we can just allocate a buffer with the size of the resource. Here is the final patch of the parent bug: https://issues.apache.org/bugzilla/attachment.cgi?id=13689
        Hide
        Scott O'Bryan added a comment -

        Cool. I think we have enough to go on. Now all someone has to do it take it on.. You wouldn't happen to be interested in attempting a patch at this would you? Seems pretty straight forward and I'd be happy to review and commit it for you. I just don't have the time right now to fix it.

        Show
        Scott O'Bryan added a comment - Cool. I think we have enough to go on. Now all someone has to do it take it on.. You wouldn't happen to be interested in attempting a patch at this would you? Seems pretty straight forward and I'd be happy to review and commit it for you. I just don't have the time right now to fix it.
        Hide
        Xiaoming Shi added a comment -

        Hi Scott,

        I'd like to, but currently I'm working on a paper deadline. I think I can take it, if no one has taken it after that

        Show
        Xiaoming Shi added a comment - Hi Scott, I'd like to, but currently I'm working on a paper deadline. I think I can take it, if no one has taken it after that
        Hide
        Scott O'Bryan added a comment -

        Sounds good Xiaoming. If there are any other takers, feel free..

        Show
        Scott O'Bryan added a comment - Sounds good Xiaoming. If there are any other takers, feel free..

          People

          • Assignee:
            Unassigned
            Reporter:
            Xiaoming Shi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development