Derby
  1. Derby
  2. DERBY-2150

Reduce use of synchronized collections in GenericLanguageConnectionContext

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.3.1.4
    • Component/s: SQL
    • Labels:
      None
    • Bug behavior facts:
      Performance

      Description

      In org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext, it is probably safe to replace some of the synchronized collections with unsynchronized ones. This should be investigated, and the unnecessary synchronization should be removed. See discussion here: http://www.nabble.com/Use-of-synchronized-containers-in-engine-code-tf2754469.html

      1. derby-2150-1a.diff
        7 kB
        Knut Anders Hatlen
      2. derby-2150-1b.diff
        8 kB
        Knut Anders Hatlen
      3. derby-2150-2a.diff
        8 kB
        Knut Anders Hatlen
      4. derby-2150-2a.stat
        0.3 kB
        Knut Anders Hatlen

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        Committed revision 497334.
        Closing the issue since all synchronized collections have been removed from GLCC.

        Show
        Knut Anders Hatlen added a comment - Committed revision 497334. Closing the issue since all synchronized collections have been removed from GLCC.
        Hide
        Knut Anders Hatlen added a comment -

        Attached another patch (2a) which removes the rest of the synchronized containers (two Hashtables) in GenericLCC. Also touched a couple of other files because the signature of copyHashtableToAIHT() was changed. Please review!

        Show
        Knut Anders Hatlen added a comment - Attached another patch (2a) which removes the rest of the synchronized containers (two Hashtables) in GenericLCC. Also touched a couple of other files because the signature of copyHashtableToAIHT() was changed. Please review!
        Hide
        Knut Anders Hatlen added a comment -

        Thank you for reviewing the patch, Dan! I changed it as you suggested and committed revision 495164.

        Show
        Knut Anders Hatlen added a comment - Thank you for reviewing the patch, Dan! I changed it as you suggested and committed revision 495164.
        Hide
        Daniel John Debrunner added a comment -

        patch 1b Looks good, minor comment on this line:

        maxActsSize = Math.max(acts.size(), maxActsSize);

        This will mean that maxActsSize (a field) is always modified, even if it isn't changing.
        Would a simple if statement be better?

        if (acts.size() > maxActsSize)
        maxActsSize = acts.size();

        Show
        Daniel John Debrunner added a comment - patch 1b Looks good, minor comment on this line: maxActsSize = Math.max(acts.size(), maxActsSize); This will mean that maxActsSize (a field) is always modified, even if it isn't changing. Would a simple if statement be better? if (acts.size() > maxActsSize) maxActsSize = acts.size();
        Hide
        Knut Anders Hatlen added a comment -

        I have uploaded a new version of the patch (1b) which preserves the
        space optimization which previously was implemented with
        Vector.capacity() and Vector.trimToSize(). I have not found any way to
        manipulate the Vectors without having synchronized on the connection
        first, so I believe it is safe to replace them with vectors.

        Derbyall and JUnit tests ran cleanly on the updated patch. Reviews
        would be appreciated. Thanks!

        Show
        Knut Anders Hatlen added a comment - I have uploaded a new version of the patch (1b) which preserves the space optimization which previously was implemented with Vector.capacity() and Vector.trimToSize(). I have not found any way to manipulate the Vectors without having synchronized on the connection first, so I believe it is safe to replace them with vectors. Derbyall and JUnit tests ran cleanly on the updated patch. Reviews would be appreciated. Thanks!
        Hide
        Knut Anders Hatlen added a comment -

        I've had a partial patch for this issue in my sandbox for a while. Attaching it here so that it won't be lost. The patch replaces four Vectors with ArrayLists. More investegation is needed in order to find out whether these changes are safe. FWIW, derbyall and the JUnit tests ran cleanly on Solaris 10 and Sun Java SE 5.0 with this patch.

        removeActivation() contains some code which is not as easily expressed with ArrayList as with Vector, since ArrayList doesn't have an equivalent to Vector.capacity().

        int capacity = acts.capacity();

        if (capacity > 20 && (capacity > 2 * acts.size()))

        { acts.trimToSize(); }

        This optimization might save space in some cases, but it doesn't seem like a very important optimization (I believe it is even less important now that DERBY-418 is fixed), so I just commented it out in the patch for now. If one really wants this optimization, it should be fairly easy to implement it by keeping track of the largest observed value of acts.size() in addActivation() and use that value instead of acts.capacity() in removeActivation().

        But first we need to find out whether it is safe to remove the synchronization...

        Show
        Knut Anders Hatlen added a comment - I've had a partial patch for this issue in my sandbox for a while. Attaching it here so that it won't be lost. The patch replaces four Vectors with ArrayLists. More investegation is needed in order to find out whether these changes are safe. FWIW, derbyall and the JUnit tests ran cleanly on Solaris 10 and Sun Java SE 5.0 with this patch. removeActivation() contains some code which is not as easily expressed with ArrayList as with Vector, since ArrayList doesn't have an equivalent to Vector.capacity(). int capacity = acts.capacity(); if (capacity > 20 && (capacity > 2 * acts.size())) { acts.trimToSize(); } This optimization might save space in some cases, but it doesn't seem like a very important optimization (I believe it is even less important now that DERBY-418 is fixed), so I just commented it out in the patch for now. If one really wants this optimization, it should be fairly easy to implement it by keeping track of the largest observed value of acts.size() in addActivation() and use that value instead of acts.capacity() in removeActivation(). But first we need to find out whether it is safe to remove the synchronization...

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development