265534 - reduce inlining of string constructors

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron:]

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.)

Comment 1

[Darin Fisher]

duplicate of bug 235104 ... want to take that one instead?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch doing what comment 3 describes

Do we want to give this a shot on the perf tests?

Comment 4

[David Baron :dbaron:]

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 6

[Benjamin Smedberg]

Comment on attachment 332363 [details] [diff] [review]
Patch doing what comment 3 describes

Give it a go on the tryserver for perf numbers?

Comment 7

[Boris Zbarsky [:bzbarsky]]

         |   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 8

[David Baron :dbaron:]

Comment on attachment 332363 [details] [diff] [review]
Patch doing what comment 3 describes

sr=dbaron

Comment 9

[Boris Zbarsky [:bzbarsky]]

Pushed as changeset 70c7932205ee.