Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.6
    • Fix Version/s: 1.7.4, 1.8-beta-1
    • Component/s: groovy-jdk
    • Labels:
      None
    • Number of attachments :
      2

      Description

      The DGM/PDGM methods

      • StringBuffer leftShift(String, Object)
      • StringBuffer leftShift(StringBuffer, Object)
      • StringBuilder leftShift(StringBuilder, Object)

      are (up to at minimum ten times) slower than they should be. Possible faster implementations are

          public static StringBuffer leftShift(String self, Object value) {
          	final CharSequence chseq;
          	if (value instanceof CharSequence)
          		chseq = (CharSequence)value;
          	else
          		chseq = String.valueOf(value);
          	final int capacity = 2 * (self.length() + chseq.length());
              return new StringBuffer(capacity).append(self).append(chseq);
          }
      
          public static StringBuffer leftShift(StringBuffer self, Object value) {
          	if (value instanceof CharSequence)
          		return self.append((CharSequence)value);
          	else
          		return self.append(value);
          }
      
          public static StringBuilder leftShift(StringBuilder self, Object value) {
          	if (value instanceof CharSequence)
          		return self.append((CharSequence)value);
          	else
          		return self.append(value);
          }
      
      1. AppendPerformance.java
        1 kB
        Alexander Veit
      2. improvedLeftShift.java
        0.8 kB
        Alexander Veit

        Activity

        Hide
        Alexander Veit added a comment -

        Seem like JIRA doesn't like source code. Now attached.

        Show
        Alexander Veit added a comment - Seem like JIRA doesn't like source code. Now attached.
        Hide
        blackdrag blackdrag added a comment -

        may I ask how you tested that? Also for code example you can use

        { followed by code followed by }

        as a starting tag and as a ending tag.

        Show
        blackdrag blackdrag added a comment - may I ask how you tested that? Also for code example you can use { followed by code followed by } as a starting tag and as a ending tag.
        Hide
        Alexander Veit added a comment -

        I noticed that leftShift(StringBuffer, Object) and leftShift(StringBuilder, Object) indirectly call String.valueOf(Object). If Object itself is a String

        {Builder,Buffer} this leads to unneccessary memory allocation. To avoid this, the String{Builder,Buffer}

        classes have dedicated append methods. So I played around with the attached Java program that indeed showed performance differences. They are probably neglectible in single user scenarios, in server applications where memory allocation can be more expensive, they may not.

        Note that the factor 2 for the capacity computation in leftShift(String, Object) is choosen according to the expandCapacity algorithm in StringBuffer. The initial buffer capacity could also be computed e.g. as self.length() + chseq.length() + 16 as in the constructor of StringBuffer.

        Thank you for your code formatting tip.

        Show
        Alexander Veit added a comment - I noticed that leftShift(StringBuffer, Object) and leftShift(StringBuilder, Object) indirectly call String.valueOf(Object). If Object itself is a String {Builder,Buffer} this leads to unneccessary memory allocation. To avoid this, the String{Builder,Buffer} classes have dedicated append methods. So I played around with the attached Java program that indeed showed performance differences. They are probably neglectible in single user scenarios, in server applications where memory allocation can be more expensive, they may not. Note that the factor 2 for the capacity computation in leftShift(String, Object) is choosen according to the expandCapacity algorithm in StringBuffer. The initial buffer capacity could also be computed e.g. as self.length() + chseq.length() + 16 as in the constructor of StringBuffer. Thank you for your code formatting tip.
        Hide
        blackdrag blackdrag added a comment -

        I repeated your performance test, but with a small modification. The time is taken outside the test method, the est method works with an empty String, StringBuilder, StringBuffer, I will not reuse the buffer by setting it to 0. Why not reuse it? Because most of the time this is not done by the user and resizing the buffer is what takes much time. Then I tested your methods against the DefaultGroovyMethods method and in case of StringBuilder against PluginDefaultMethods in 1.6. The result is, that in case of String, your method seems to be slower by a around 90%, for StringBuffer your method was slower by around 60%. Only your StringBuilder methods seems to be faster.. Improvement nearly 70%. AS a side note, the Groovy implementation takes alsmost the same time for all methods, while your String method is 2 times slower than your StringBuilder method, StringBuffer is slower by around 100%

        I just tested reusing StringBuffer, and looks like the speed difference will not decrease.If I amke the same test for StringBuilder, then your method is around 300% faster

        So in the end I guess we can take your StringBuilder version, but the others won't help.

        Show
        blackdrag blackdrag added a comment - I repeated your performance test, but with a small modification. The time is taken outside the test method, the est method works with an empty String, StringBuilder, StringBuffer, I will not reuse the buffer by setting it to 0. Why not reuse it? Because most of the time this is not done by the user and resizing the buffer is what takes much time. Then I tested your methods against the DefaultGroovyMethods method and in case of StringBuilder against PluginDefaultMethods in 1.6. The result is, that in case of String, your method seems to be slower by a around 90%, for StringBuffer your method was slower by around 60%. Only your StringBuilder methods seems to be faster.. Improvement nearly 70%. AS a side note, the Groovy implementation takes alsmost the same time for all methods, while your String method is 2 times slower than your StringBuilder method, StringBuffer is slower by around 100% I just tested reusing StringBuffer, and looks like the speed difference will not decrease.If I amke the same test for StringBuilder, then your method is around 300% faster So in the end I guess we can take your StringBuilder version, but the others won't help.
        Hide
        Alexander Veit added a comment -

        I resized the buffer to factor out the time for it's allocation. It seems to me the common case that a receiving buffer is already allocated when append is called. setLength(0) does only set the internal buffer pointer to 0 and thus is very efficient.

        In all cases, performance improvements are negligible if a java.lang.String is appended to the receiving buffer, since String.valueOf(String) is a rather trivial operation. For other objects that are being appended String.valueOf(Object) may be expensive. This is especially true for the mutable classes StringBuffer and StringBuilder whose data need to be copied to a temporary String.

        These are the numbers i got for StringBuilder (for StringBuffer they are almost the same):

        StringBuilder#append((Object)StringBuilder(10000)
        Time = 2692 ms (* 5.6)
        StringBuilder#append((CharSequence)StringBuilder(10000)
        Time = 483 ms

        StringBuilder#append((Object)StringBuilder(50000)
        Time = 14366 ms (* 5.4)
        StringBuilder#append((CharSequence)StringBuilder(50000)
        Time = 2654 ms

        StringBuilder#append((Object)StringBuilder(100000)
        Time = 39924 ms (* 6.4)
        StringBuilder#append((CharSequence)StringBuilder(100000)
        Time = 6228 ms

        StringBuilder#append((Object)StringBuilder(200000)
        Time = 102777 ms (* 2.9)
        StringBuilder#append((CharSequence)StringBuilder(200000)
        Time = 35021 ms

        The performance degradation in leftShift(String self, Object value) is probably due to the allocation of a buffer of the double size. I did this because I assumed that ordinary string concatenation would first call leftShift(String self, Object value) and then leftShift(StringBuffer self, Object value) for subsequent terms. But this assumption may be false.

        I'm not sure what the conclusion of our two tests should be.

        Show
        Alexander Veit added a comment - I resized the buffer to factor out the time for it's allocation. It seems to me the common case that a receiving buffer is already allocated when append is called. setLength(0) does only set the internal buffer pointer to 0 and thus is very efficient. In all cases, performance improvements are negligible if a java.lang.String is appended to the receiving buffer, since String.valueOf(String) is a rather trivial operation. For other objects that are being appended String.valueOf(Object) may be expensive. This is especially true for the mutable classes StringBuffer and StringBuilder whose data need to be copied to a temporary String. These are the numbers i got for StringBuilder (for StringBuffer they are almost the same): StringBuilder#append((Object)StringBuilder(10000) Time = 2692 ms (* 5.6) StringBuilder#append((CharSequence)StringBuilder(10000) Time = 483 ms StringBuilder#append((Object)StringBuilder(50000) Time = 14366 ms (* 5.4) StringBuilder#append((CharSequence)StringBuilder(50000) Time = 2654 ms StringBuilder#append((Object)StringBuilder(100000) Time = 39924 ms (* 6.4) StringBuilder#append((CharSequence)StringBuilder(100000) Time = 6228 ms StringBuilder#append((Object)StringBuilder(200000) Time = 102777 ms (* 2.9) StringBuilder#append((CharSequence)StringBuilder(200000) Time = 35021 ms The performance degradation in leftShift(String self, Object value) is probably due to the allocation of a buffer of the double size. I did this because I assumed that ordinary string concatenation would first call leftShift(String self, Object value) and then leftShift(StringBuffer self, Object value) for subsequent terms. But this assumption may be false. I'm not sure what the conclusion of our two tests should be.
        Hide
        blackdrag blackdrag added a comment -

        as I said, the test didin't seem to change much in the factores between your and the actual implementation when I use a pre allocated buffer or not. Ordinary String concatenation as in "foo"+bar is done by the plus method, which performs bad, because it creates a new String in the end, instead of internally using a String buffer for multiple parts. But that is due to he nature of Groovy and can't be changed easily.

        As a conclusion I would use your StringBuilder implementation, but keep the old ones for String and StringBuffer. Do you agree?

        Show
        blackdrag blackdrag added a comment - as I said, the test didin't seem to change much in the factores between your and the actual implementation when I use a pre allocated buffer or not. Ordinary String concatenation as in "foo"+bar is done by the plus method, which performs bad, because it creates a new String in the end, instead of internally using a String buffer for multiple parts. But that is due to he nature of Groovy and can't be changed easily. As a conclusion I would use your StringBuilder implementation, but keep the old ones for String and StringBuffer. Do you agree?
        Hide
        Alexander Veit added a comment -

        Agreed.

        Show
        Alexander Veit added a comment - Agreed.
        Show
        Alexander Veit added a comment - FYI: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6734175
        Hide
        Guillaume Laforge added a comment -

        Taken into account the StringBuilder variant which was the one with a real gain as discussed.

        Show
        Guillaume Laforge added a comment - Taken into account the StringBuilder variant which was the one with a real gain as discussed.

          People

          • Assignee:
            Guillaume Laforge
            Reporter:
            Alexander Veit
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: