271817 - do_CreateInstance causes unnecessary bloat

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Bernard Alleysson]

nsCreateInstanceBy* have 3 members and they are not needed most of the times.
They can be removed by providing new "nsCreateInstanceBy" classes for the cases
where mOuter and mErrorPtr are not used, thus removing code bloat caused by the
inline constructors.

Comment 1

[Bernard Alleysson]

Attachment: patch 1

Provide nsCreateInstanceBy*WithError, nsCreateInstanceBy*WithOuter,
nsCreateInstanceBy*WithOuterError to remove some code bloat caused by the
inline constructors

There are about 1400 do_CreateInstance() in the source
The gain is maximal when do_CreateInstance(contractid) or
do_CreateInstance(cid) are used.

Comment 2

[Bernard Alleysson]

Comment on attachment 167087 [details] [diff] [review]
patch 1

Brian, would you review the patch ?
It is similar to what you've done for bug	 264456
(nsGetServiceBy*WithError)

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 167087 [details] [diff] [review]
patch 1

>--- nsComponentManagerUtils.cpp	24 Nov 2004 23:12:18 -0000	1.10
>+++ nsComponentManagerUtils.cpp	26 Nov 2004 07:07:59 -0000
>@@ -271,23 +329,23 @@ nsGetServiceByCIDWithError::operator()( 
> }
> 
> nsresult
> nsGetServiceByContractID::operator()( const nsIID& aIID, void** aInstancePtr ) const
> {
>     nsresult status = CallGetService(mContractID, aIID, aInstancePtr);
>     if ( NS_FAILED(status) )
>         *aInstancePtr = 0;
>-    
>+
>     return status;
> }

It's generally better to avoid unneeded whitespace changes where possible.

> 
> nsresult
> nsGetServiceByContractIDWithError::operator()( const nsIID& aIID, void** aInstancePtr ) const
> {
>     nsresult status = CallGetService(mContractID, aIID, aInstancePtr);
>     if ( NS_FAILED(status) )
>         *aInstancePtr = 0;
>-    
>+
>     if ( mErrorPtr )
>         *mErrorPtr = status;
>     return status;
> }

Same.

Looks ok otherwise, but I'd like to see what the code size numbers are first. 
If you can run them that would be great, otherwise, I can get to it later today
or tomorrow.

Comment 4

[Bernard Alleysson]

I agree with whitespaces changes, I'll be more careful in the future.

There's almost no code size savings on Windows with VC7 (-512 bytes with the
patch), but looking at numbers for similar bug (bug 264456) it should be an
improvement on Linux (I can't see any downside with this patch, anyway).

With this patch it is now possible to make all nsCreateInstance* operator() not
virtual but I don't think it's critical (do_CreateInstance is supposed to be
slow ?, as opposed to do_GetInterface), and it is tedious to write, though trivial.

I can't get code size numbers for Linux and it would be great if you could have
some numbers (maybe including patch for bug 271845 too). Let me how it goes,
either way, and I'll ask darin, who reviewed your recent changes, for superreview.

Thanks

Comment 5

[Bernard Alleysson]

Sorry for the spam, just to make sure you receive bugmail (see esp my previous
comment on codesize)

Comment 6

[Bernard Alleysson]

Comment on attachment 167087 [details] [diff] [review]
patch 1

I don't have numbers for Linux, but it saves a few bytes on Windows so I'm
pretty confident about the patch

Comment 7

[Darin Fisher]

Comment on attachment 167087 [details] [diff] [review]
patch 1

So, this looks okay, but you're basically increasing the codesize of the XPCOM
library, assuming that the small savings at all of the call sites will make up
for the increase.  I'd also like to see codesize savings first before approving
this patch.

Comment 8

[Bernard Alleysson]

(In reply to comment #7)
> ... the small savings at all of the call sites will make up for the increase.  

Exactly.

There are #1300 do_CreateInstance().

Of these 1300, only 2 use of the "WithOuter" variant, here:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#2803
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFTPChannel.cpp#120

So basicly, if they were changed to use CallCreateInstance() directly, "mOuter"
could be totally removed, the patch could be simpler, thus recovering some
xpcom_core code size increase from the patch. But I didn't want to remove any
functionality.

Of the remaining instances, it looks like 50% of them doesn't use the error
variant, do_CI(..., &rv), so there's some saving here also.

Comment 9

[Bernard Alleysson]

Attachment: codesize on Windows

xpcom_core.dll is 3 kb bigger, but other dlls are smaller, and overall there's
1 kb code size increase

Comment 10

[Darin Fisher]

Comment on attachment 167087 [details] [diff] [review]
patch 1

so, I don't think this patch is worth it.  i recommend WONTFIX on this bug, but if someone else wants to SR the patch, then ok.

Comment 11

[Doug Turner (:dougt)]

was there any perf. gain?