Closed Bug 584299 Opened 12 years ago Closed 12 years ago

Inline (again) string constructors and destructors

Categories

(Core :: XPCOM, defect, P1)

Product:
Core
Component:
XPCOM
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b4

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

OK, with that change
8.91 KB, patch
benjamin
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
Attached patch Like so (obsolete) — — Splinter Review 
We took these out of line to reduce codesize, and performance was a wash on our macrobenchmarks.  Inlining them wins us 10% on getAttribute microbenchmarks nowadays.... which means in practice around 25ns per call to getAttribute on my hardware.

The ifdefs aren't pretty; I didn't have any better ideas, though.
Attachment #462700 - Flags: review?(benjamin)
Priority: -- → P1
Summary: Inline (again) string constructors and destructors → [FIX]Inline (again) string constructors and destructors
Comment on attachment 462700 [details] [diff] [review]
Like so

remove the GCC 3.3 workaround: I'd prefer to break OS/2 than to have ifdefed constructor styles.
Attachment #462700 - Flags: review?(benjamin) → review-
Attachment #462700 - Attachment is obsolete: true
Attachment #462865 - Flags: review?(benjamin)
Comment on attachment 462865 [details] [diff] [review]
OK, with that change

Gah, ok. We can get rid of the ifdefs as soon as we require libxul (which will be soon after branch). File a followup, please.
Attachment #462865 - Flags: review?(benjamin) → review+
Is it possible to make further savings by moving the destructor so that classes such as nsDependentString that don't need it, don't have one?
In the current string code, nsDependentString is really only a constructor and not a useful static type, I think; I don't recall anything that prevents one from being assigned-to.
Attachment #462865 - Flags: approval2.0?
How would libxul let us get rid of the ifdefs?  We'd still need to add more includes for that ctor body to compile, right?
Summary: [FIX]Inline (again) string constructors and destructors → Inline (again) string constructors and destructors
Whiteboard: [need approval]
Yes, and I'm perfectly fine with that.
Ah, ok.  Gotcha.
Filed bug 584726.
Attachment #462865 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/48a78e198bfd

Seems to give a .5-1% codesize increase and 1-3% overall Dromaeo performance win.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b4
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.