608914 - Move AppendFloat/AppendInt up to nsAString

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Unless there's a _really_ good reason to not have them there, we should do this.  Currently we end up with silly temporary strings and string copies, even though both caller and callee have nsStrings.

The other option is to move perf-sensitive stuff that might need to do those away from XPCOM interfaces that return strings, but that's a pretty big project.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the nsString overloads of AppendInt and move AppendFloat up to nsAString.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of some temporary strings now that we can AppendFloat to an nsAString.

Comment 3

[Benjamin Smedberg]

Comment on attachment 487809 [details] [diff] [review]
part 1.  Remove the nsString overloads of AppendInt and move AppendFloat up to nsAString.

I presume we have tests covering this stuff already?

Comment 4

[Boris Zbarsky [:bzbarsky]]

We have TestStrings.cpp, but that's only compiled in non-libxul builds, of course...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 487810 [details] [diff] [review]
part 2.  Get rid of some temporary strings now that we can AppendFloat to an nsAString.

r=dbaron

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: roll-up for approval

Should be a pretty safe perf win.

Comment 7

[[e-mail]]

Comment on attachment 487810 [details] [diff] [review]
part 2.  Get rid of some temporary strings now that we can AppendFloat to an nsAString.

>-      tmpStr.AppendInt(NS_GET_R(color), 10);
>-      tmpStr.Append(comma);
>-      tmpStr.AppendInt(NS_GET_G(color), 10);
>-      tmpStr.Append(comma);
>-      tmpStr.AppendInt(NS_GET_B(color), 10);
>+      aResult.AppendInt(NS_GET_R(color), 10);
>+      aResult.Append(comma);
>+      aResult.AppendInt(NS_GET_G(color), 10);
>+      aResult.Append(comma);
>+      aResult.AppendInt(NS_GET_B(color), 10);
[I wonder why these explicitly state the default radix. This is slightly suboptimal for debug builds but presumably opt builds optimise it away.]

Comment 8

[Boris Zbarsky [:bzbarsky]]

I have no idea.  I was trying for minimal (hence safest) change which would fix the perf issues I was seeing... ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed:

  http://hg.mozilla.org/mozilla-central/rev/f61c6c4ba5be
  http://hg.mozilla.org/mozilla-central/rev/51125803eb31

Comment 10

[Alfred Kayser]

Note, Modified_cnvtf allocates a temporary buffer the size of the buffer passed in as argument, which we know is limited to 40 characters (as it is only used with nsTSubstring.cpp itself), so one could use a stack buffer instead.
Added this comment to bug 608915 which is about optimizing AppendFloat.

Comment 11

[Dave Townsend [:mossop]]

Backed out the patch for being in the range of bug 615736

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/48657c9d5788
  http://hg.mozilla.org/mozilla-central/rev/c5136e816bde

Tree is green.

Comment 13

[Jonathan Kew [:jfkthame]]

Adding dev-doc-needed, as it looks like MDN documents these on nsString but not nsAString.