641398 - group character append inside a loop

Status: NEW

(Core :: General, enhancement)

Description

[aliang]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: firefox-4.0b12.source

I found around 25 cases in firefox-4.0b12.source which append characters one by one inside a loop. Here I pick five of them and propose patches for them to group them into one append statement. With those patches, the performance should be improved because the number of append function call is significantly reduced.

around ./firefox-4.0b12.source/layout/mathml/nsMathMLmpaddedFrame.cpp:199
  // get the number
  PRBool gotDot = PR_FALSE, gotPercent = PR_FALSE;
+  PRUint32 appendOffset = i;
  for (; i < stringLength; i++) {
    PRUnichar c = aString[i];
    if (gotDot && c == '.') {
      // error - two dots encountered
      aSign = NS_MATHML_SIGN_INVALID;
      return PR_FALSE;
    }

    if (c == '.')
      gotDot = PR_TRUE;
    else if (!nsCRT::IsAsciiDigit(c)) {
      break;
    }
-    number.Append(c);
  }
+  number.Append(&aString[appendOffset], i - appendOffset);

around ./firefox-4.0b12.source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp:2157
        if (!nameFromURL.IsEmpty())
        {
            // Unescape the file name (GetFileName escapes it)
            NS_UnescapeURL(nameFromURL);
            PRUint32 nameLength = 0;
            const char *p = nameFromURL.get();
+            PRUint32 appendLength = 0;
            for (;*p && *p != ';' && *p != '?' && *p != '#' && *p != '.'
                 ;p++)
            {
                if (nsCRT::IsAsciiAlpha(*p) || nsCRT::IsAsciiDigit(*p)
                    || *p == '.' || *p == '-' ||  *p == '_' || (*p == ' '))
                {
-                    fileName.Append(PRUnichar(*p));
+                    appendLength++;
                    if (++nameLength == kDefaultMaxFilenameLength)
                    {
                        // Note:
                        // There is no point going any further since it will be
                        // truncated in CalculateUniqueFilename anyway.
                        // More importantly, certain implementations of
                        // nsILocalFile (e.g. the Mac impl) might truncate
                        // names in undesirable ways, such as truncating from
                        // the middle, inserting ellipsis and so on.
                        break;
                    }
                }
            }
+            fileName.Append((PRUnichar*)p, appendLength);
        }

around ./firefox-4.0b12.source/content/mathml/contentsrc/nsMathMLElement.cpp:278
  // Gather up characters that make up the number
  PRBool gotDot = PR_FALSE;
+  PRUint32 appendOffset = i;
  for ( ; i < stringLength; i++) {
    c = str[i];
    if (gotDot && c == '.')
      return PR_FALSE;  // two dots encountered
    else if (c == '.')
      gotDot = PR_TRUE;
    else if (!nsCRT::IsAsciiDigit(c)) {
      str.Right(unit, stringLength - i);
      // some authors leave blanks before the unit, but that shouldn't
      // be allowed, so don't CompressWhitespace on 'unit'.
      break;
    }
-    number.Append(c);
  }
+  number.Append(&aString[appendOffset], i - appendOffset);

around ./firefox-4.0b12.source/content/base/src/nsGenericDOMDataNode.cpp:446
    while (cp < end) {
      PRUnichar ch = *cp++;
+      PRUint32 appendLength = 0;
+      const PRUnichar* cpStart = cp;
+      while (ch != '&' && ch != '<' && ch != '>' && ch >= ' ' && ch < 127
+          && cp < end) {
+        ch = *cp++;
+        appendLength++;
+      }
+      aBuf.Append(cpStart, appendLength);
      if (ch == '&') {
        aBuf.AppendLiteral("&amp;");
      } else if (ch == '<') {
        aBuf.AppendLiteral("&lt;");
      } else if (ch == '>') {
        aBuf.AppendLiteral("&gt;");
      } else if ((ch < ' ') || (ch >= 127)) {
        char buf[10];
        PR_snprintf(buf, sizeof(buf), "\\u%04x", ch);
        AppendASCIItoUTF16(buf, aBuf);
      } else {
        aBuf.Append(ch);
      }
    }

around ./firefox-4.0b12.source/content/base/src/nsGenericDOMDataNode.cpp:466
    while (cp < end) {
      PRUnichar ch = *cp++;;
+      const PRUnichar* cpStart = cp;
+      while (ch != '&' && ch != '<' && ch != '>' && ch >= ' ' && ch < 127
+          && cp < end) {
+        ch = *cp++;
+        appendLength++;
+      }
+      aBuf.Append(cpStart, appendLength);
      if (ch == '&') {
        aBuf.AppendLiteral("&amp;");
      } else if (ch == '<') {
        aBuf.AppendLiteral("&lt;");
      } else if (ch == '>') {
        aBuf.AppendLiteral("&gt;");
      } else if ((ch < ' ') || (ch >= 127)) {
        char buf[10];
        PR_snprintf(buf, sizeof(buf), "\\u%04x", ch);
        AppendASCIItoUTF16(buf, aBuf);
      } else {
        aBuf.Append(ch);
      }
    }

These patches are inspired by the patch for bug 311566, which also groups character append inside a loop.

Reproducible: Didn't try

Comment 1

[Justin Dolske [:Dolske]]

(Bah, khuey beat me to the move :)

Thanks for looking into this.

You may want to read through https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch, it's generally preferred to use bugzilla attachments in that format, and it also explains a bit of the process if this is your first time through it. :)

Comment 2

[aliang]

So should I attach some diff files?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Fwiw, other than the nsGenericDOMDataNode code thise is not perf-sensitive, so not worth the extra compexity.

For nsGenericDOMDataNode, I'd like to see some before/after numbers.