Closed
Bug 584299
Opened 13 years ago
Closed 13 years ago
Inline (again) string constructors and destructors
Categories
(Core :: XPCOM, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla2.0b4
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(1 file, 1 obsolete file)
8.91 KB,
patch
|
benjamin
:
review+
benjamin
:
approval2.0+
|
Details | Diff | 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)
![]() |
Assignee | |
Updated•13 years ago
|
Priority: -- → P1
![]() |
Assignee | |
Updated•13 years ago
|
Summary: Inline (again) string constructors and destructors → [FIX]Inline (again) string constructors and destructors
Comment 1•13 years ago
|
||
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-
![]() |
Assignee | |
Comment 2•13 years ago
|
||
Attachment #462700 -
Attachment is obsolete: true
Attachment #462865 -
Flags: review?(benjamin)
Comment 3•13 years ago
|
||
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+
Comment 4•13 years ago
|
||
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?
Comment 5•13 years ago
|
||
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.
![]() |
Assignee | |
Updated•13 years ago
|
Attachment #462865 -
Flags: approval2.0?
![]() |
Assignee | |
Comment 6•13 years ago
|
||
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?
![]() |
Assignee | |
Updated•13 years ago
|
Summary: [FIX]Inline (again) string constructors and destructors → Inline (again) string constructors and destructors
Whiteboard: [need approval]
Comment 7•13 years ago
|
||
Yes, and I'm perfectly fine with that.
![]() |
Assignee | |
Comment 8•13 years ago
|
||
Ah, ok. Gotcha.
![]() |
Assignee | |
Comment 9•13 years ago
|
||
Filed bug 584726.
Updated•13 years ago
|
Attachment #462865 -
Flags: approval2.0? → approval2.0+
![]() |
Assignee | |
Updated•13 years ago
|
Whiteboard: [need approval] → [need landing]
![]() |
Assignee | |
Comment 10•13 years ago
|
||
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: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b4
Updated•2 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•