Closed Bug 265534 Opened 19 years ago Closed 15 years ago

reduce inlining of string constructors


(Core :: XPCOM, defect)

Not set





(Reporter: dbaron, Assigned: bzbarsky)



(Keywords: memory-footprint)


(1 file)

String class constructors, particularly those that take parameters, should be
inlined much less.  This could be a *huge* codesize improvement (and thus
probably performance improvement).

(While doing this, it might help to make nsAutoString128, nsAutoString256, etc.,
so that nsFixedString's constructors can be protected.)
duplicate of bug 235104 ... want to take that one instead?
Blocks: 235104
Some data:

Attachment 245232 [details] [diff] reduced codesize of gklayout by about 40KB.

I tried taking all the other ctors of nsTSubstring_CharT our of line, and that only got me another 13KB in gklayout (compiling with -Os, stripped .so size).

Then I tried taking the ctors of nsTFixedString_CharT out of line, and that got me another 58KB or so.

This is all on a baseline size of about 5.3MB for gklayout.
QA Contact: string
Do we want to give this a shot on the perf tests?
Attachment #332363 - Flags: superreview?(dbaron)
Attachment #332363 - Flags: review?(benjamin)
One other thing worth experimenting with is seeing whether it's worth having constructors that are the base class constructors used by derived classes should be inline (and protected) so that we get a single constructor call rather than a whole bunch.
Comment on attachment 332363 [details] [diff] [review]
Patch doing what comment 3 describes

Give it a go on the tryserver for perf numbers?
Attachment #332363 - Flags: review?(benjamin) → review+
         |   mozilla-central  |     with patch
Linux Ts |      1556.95       |    1570.74
Linux Tp |       452.06       |     448.92
Mac Ts   | 1238.79 or 1240.58 |  1232.47 or 1327.84
Mac Tp   | 313.32 or 314.50   |  313.11 or 315.75

The Windows tryserver decided not to build my baseline for some reason (though it built things that came both before and after it on the other tryservers), but the with-patch numbers were:

Ts: 1397.32
Tp: 442.30

These are not statistically different from the numbers for the other patches being tried.

So it looks to me like this is a wash performance-wise.
Comment on attachment 332363 [details] [diff] [review]
Patch doing what comment 3 describes

Attachment #332363 - Flags: superreview?(dbaron) → superreview+
Pushed as changeset 70c7932205ee.
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 451278
Keywords: footprint
Assignee: dbaron → bzbarsky
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.