188224 - excessive inlining in string code?

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We should look for places where we're inlining too much in string code.  I made
some comments in my review of a patch on bug 162017 that never landed.

However, what really worries me is that the one line checkin for bug 133754,
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/embedding/components/windowwatcher/src&command=DIFF_FRAMESET&file=nsWindowWatcher.cpp&rev1=1.68&rev2=1.69&root=/cvsroot
caused a 320 byte increase in code size:
----------- Output from codesize diff log ------------- 
  Overall Change in Size
  	Total:	       +320 (+324/-4)
  	Code:	       +320 (+324/-4)
  	Data:	         +0 (+0/+0)
  
  libembedcomponents.so
  	Total:	       +320 (+324/-4)
  	Code:	       +320 (+324/-4)
  	Data:	         +0 (+0/+0)
  	       +320 (+324/-4)	T (CODE)
  		       +320 (+324/-4)	UNDEF:libembedcomponents.so:T
  			       +324	nsWindowWatcher::OpenWindowJS(nsIDO...
  			         -4	NS_NewControllerCommandManager(nsIControllerCommandManager **)
----------- End Output from codesize diff log ---------

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I was referring to bug 162017 comment 20.)

Comment 2

[Alec Flett]

to be honest, that one line checkin should have resulted in bloat, because we're
now creating a NS_ConvertUTF8toUCS2.. 320 bytes does seem like a lot though. 

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's 320 bytes of code size, not data size.  (And anyway, any data size
measurement should be looking at the heap rather than the stack, and not
emphasizing transient objects.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I haven't been able to reproduce this steep a code size penalty.  I tried a few
simple tests on libhtmlpars.so:

526844 - original
522381 - after un-inlining ctor nsDependent[C]String
521722 - after un-inlining nsAFlat[C]String::get
521594 - after un-inlining ctor nsXPIDL[C]String

The code size gain for adding new strings seemed to vary.  Optimization seems to
do weird things, too.

Comment 5

[Alec Flett]

this actually brings me back to a warning from a long LONG time ago that John
McMullen once warned people about: he claimed that the body of any virtual
functions that was declared inline would be included with every single object
file generated

i.e. he claimed that if in nsString.h I said:

virtual const PRUnichar* get() { return mBuf; }

then the compiled body of this function would be included in the .obj for every
single .cpp file that #included nsString.h 

I had kind of scoffed at it at the time and said "how COULD that be true?" but
maybe its the only way for a compiler to guarantee that it has a pointer to a
function for that function body to put in a vtable, since it doesn't know at
compile time if that function will or wont be called? I dunno. I might be able
to discover something with codesighs (or at least msmap2tsv and related tools)
and I'll look into it, but I'm curious if anyone else has heard anything like this?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Appears to still be valid with the new string code, given bug 345517 comment 31.

Comment 7

[Benjamin Smedberg]

I believe we re-profiled this after 2006, but in any case I don't think this is worth actively tracking.