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

        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.
        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.
        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,
        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.
        Hide
        Kristian Waagan added a comment -

        Closing issue.

        Show
        Kristian Waagan added a comment - Closing issue.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development