Derby
  1. Derby
  2. DERBY-4661

Reduce size of encoding buffer for short character values

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.5.3.1, 10.6.2.1, 10.7.1.1
    • Component/s: JDBC
    • Labels:
    • Environment:
      Inserts using setXStream(int, Reader/InputStream, int/long) for short values on character columns
    • Bug behavior facts:
      Performance

      Description

      When inserting character values Derby converts from Java char to an on-disk encoding of UTF-8. To to this, the user stream is read and the resulting bytes after conversion are placed in a "translation buffer". The default size of the buffer is 32 KB. When inserting a lot of short values, the pressure on the Java garbage collector is unnecessary high and the allocation/GC also causes a somewhat higher CPU usage.

      This effect of this issue can easily be reduced by sizing the buffer in the appropriate cases.

      1. derby-4661-1a-reduce_encoding_bz.diff
        5 kB
        Kristian Waagan
      2. derby-4661-1a-reduce_encoding_bz.stat
        0.4 kB
        Kristian Waagan
      3. derby-4661-1b-reduce_encoding_bz.diff
        8 kB
        Kristian Waagan
      4. derby-4661-1b-reduce_encoding_bz.diff
        8 kB
        Kristian Waagan

        Activity

        Kristian Waagan created issue -
        Hide
        Kristian Waagan added a comment -

        Attaching patch 1a.

        • iapi/types/ReaderToUTF8Stream
          The actual fix. Note the special case when the value length is zero. To avoid the issue for shorter header lengths in the future, I used Math.max instead of handling valueLength == 0 specifically.
        • other classes
          Added StreamHeaderGenerater.getMaxHdrLength() and made SQLClob use it.

        Some simple tests showed a performance improvement of around 30%. Real world workloads will not see such a gain, but the fix may help heavily loaded servers somewhat where users are inserting small data values using the streaming classes.

        Regression tests passed.
        Patch ready for review.

        Show
        Kristian Waagan added a comment - Attaching patch 1a. iapi/types/ReaderToUTF8Stream The actual fix. Note the special case when the value length is zero. To avoid the issue for shorter header lengths in the future, I used Math.max instead of handling valueLength == 0 specifically. other classes Added StreamHeaderGenerater.getMaxHdrLength() and made SQLClob use it. Some simple tests showed a performance improvement of around 30%. Real world workloads will not see such a gain, but the fix may help heavily loaded servers somewhat where users are inserting small data values using the streaming classes. Regression tests passed. Patch ready for review.
        Kristian Waagan made changes -
        Field Original Value New Value
        Attachment derby-4661-1a-reduce_encoding_bz.diff [ 12444487 ]
        Attachment derby-4661-1a-reduce_encoding_bz.stat [ 12444488 ]
        Kristian Waagan made changes -
        Issue & fix info [Patch Available]
        Hide
        Knut Anders Hatlen added a comment -

        This looks like a useful improvement. A couple of minor comments:

        I think the code would be easier to follow if some of the magic numbers were replaced by constants or expressions. In particular:

        • the number 10922 could be replaced by bz/3. That would make it clearer where the number came from, and it would reduce the likelihood of bugs being introduced if we change the default buffer size later.
        • using a constant (e.g., READ_BUFFER_RESERVATION) instead of the number 6 in ReaderToUTF8Stream's constructor and in fillBuffer() would make the relation between those methods easier to see.

        Perhaps it would be better to call the new method in StreamHeaderGenerator getMaxHeaderLength()?

        The call to LimitReader.setLimit() in the lengthless constructor was made redundant by the patch. Should it be removed?

        Show
        Knut Anders Hatlen added a comment - This looks like a useful improvement. A couple of minor comments: I think the code would be easier to follow if some of the magic numbers were replaced by constants or expressions. In particular: the number 10922 could be replaced by bz/3. That would make it clearer where the number came from, and it would reduce the likelihood of bugs being introduced if we change the default buffer size later. using a constant (e.g., READ_BUFFER_RESERVATION) instead of the number 6 in ReaderToUTF8Stream's constructor and in fillBuffer() would make the relation between those methods easier to see. Perhaps it would be better to call the new method in StreamHeaderGenerator getMaxHeaderLength()? The call to LimitReader.setLimit() in the lengthless constructor was made redundant by the patch. Should it be removed?
        Hide
        Kristian Waagan added a comment -

        Attached patch 1b.

        Thanks for reviewing the patch, Knut Anders.

        I have addressed your suggestions, and I also made a few other changes (size logic, fixed two typos).
        Regression tests passed (suites.All).

        Patch ready for a second review.

        Show
        Kristian Waagan added a comment - Attached patch 1b. Thanks for reviewing the patch, Knut Anders. I have addressed your suggestions, and I also made a few other changes (size logic, fixed two typos). Regression tests passed (suites.All). Patch ready for a second review.
        Kristian Waagan made changes -
        Attachment derby-4661-1b-reduce_encoding_bz.diff [ 12445046 ]
        Hide
        Knut Anders Hatlen added a comment -

        Thanks for making these changes, Kristian. The 1b patch looks good to me.

        I assume this part of the patch was unintended, though:

        Property changes on: java/engine/org/apache/derby/iapi/types
        ___________________________________________________________________
        Added: svn:ignore
        + .ReaderToUTF8Stream.java.swp

        Show
        Knut Anders Hatlen added a comment - Thanks for making these changes, Kristian. The 1b patch looks good to me. I assume this part of the patch was unintended, though: Property changes on: java/engine/org/apache/derby/iapi/types ___________________________________________________________________ Added: svn:ignore + .ReaderToUTF8Stream.java.swp
        Hide
        Kristian Waagan added a comment -

        Yes, that change wasn't intended (my IDE did that for me).

        Uploaded a new rev of patch 1b, and committed it to trunk with revision 948069.

        Thanks,

        Show
        Kristian Waagan added a comment - Yes, that change wasn't intended (my IDE did that for me). Uploaded a new rev of patch 1b, and committed it to trunk with revision 948069. Thanks,
        Kristian Waagan made changes -
        Attachment derby-4661-1b-reduce_encoding_bz.diff [ 12445462 ]
        Kristian Waagan made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Issue & fix info [Patch Available]
        Fix Version/s 10.7.0.0 [ 12314971 ]
        Resolution Fixed [ 1 ]
        Hide
        Kristian Waagan added a comment -

        Backported to the 10.5 branch with revision 951363.
        Backported to the 10.6 branch with revision 951362.

        Show
        Kristian Waagan added a comment - Backported to the 10.5 branch with revision 951363. Backported to the 10.6 branch with revision 951362.
        Kristian Waagan made changes -
        Fix Version/s 10.5.3.1 [ 12314182 ]
        Fix Version/s 10.6.1.1 [ 12314973 ]
        Hide
        Kristian Waagan added a comment -

        Closing issue.

        Show
        Kristian Waagan added a comment - Closing issue.
        Kristian Waagan made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Kathey Marsden made changes -
        Fix Version/s 10.6.2.0 [ 12315342 ]
        Fix Version/s 10.6.1.1 [ 12314973 ]
        Knut Anders Hatlen made changes -
        Fix Version/s 10.6.2.1 [ 12315343 ]
        Fix Version/s 10.6.2.0 [ 12315342 ]
        Rick Hillegas made changes -
        Affects Version/s 10.7.1.1 [ 12315564 ]
        Affects Version/s 10.7.1.0 [ 12314971 ]
        Fix Version/s 10.7.1.1 [ 12315564 ]
        Fix Version/s 10.7.1.0 [ 12314971 ]
        Gavin made changes -
        Workflow jira [ 12510887 ] Default workflow, editable Closed status [ 12800250 ]

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Kristian Waagan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development