Closed Bug 608914 Opened 9 years ago Closed 9 years ago

Move AppendFloat/AppendInt up to nsAString

Categories

(Core :: String, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files)

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.
Attachment #487810 - Flags: review?(dbaron)
Attachment #487809 - Flags: review?(benjamin)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
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?
Attachment #487809 - Flags: review?(benjamin) → review+
We have TestStrings.cpp, but that's only compiled in non-libxul builds, of course...
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
Attachment #487810 - Flags: review?(dbaron) → review+
Should be a pretty safe perf win.
Attachment #493896 - Flags: approval2.0?
Whiteboard: [need review] → [need approval]
Attachment #493896 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
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.]
I have no idea.  I was trying for minimal (hence safest) change which would fix the perf issues I was seeing... ;)
Pushed:

  http://hg.mozilla.org/mozilla-central/rev/f61c6c4ba5be
  http://hg.mozilla.org/mozilla-central/rev/51125803eb31
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
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.
Backed out the patch for being in the range of bug 615736
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [need landing]
Pushed:
  http://hg.mozilla.org/mozilla-central/rev/48657c9d5788
  http://hg.mozilla.org/mozilla-central/rev/c5136e816bde

Tree is green.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Adding dev-doc-needed, as it looks like MDN documents these on nsString but not nsAString.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.