do_CreateInstance causes unnecessary bloat

RESOLVED WONTFIX

Status

()

Core
XPCOM
RESOLVED WONTFIX
14 years ago
6 years ago

People

(Reporter: Bernard Alleysson, Unassigned)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 167087 [details] [diff] [review]
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.
(Reporter)

Comment 2

14 years ago
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)
Attachment #167087 - Flags: review?(bryner)
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.
Attachment #167087 - Flags: review?(bryner) → review+
(Reporter)

Comment 4

14 years ago
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
(Reporter)

Comment 5

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

Comment 6

14 years ago
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
Attachment #167087 - Flags: superreview?(darin)

Comment 7

14 years ago
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.
(Reporter)

Comment 8

14 years ago
(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.
(Reporter)

Comment 9

14 years ago
Created attachment 168968 [details]
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

12 years ago
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.
Attachment #167087 - Flags: superreview?(darin)

Comment 11

12 years ago
was there any perf. gain?
QA Contact: xpcom
Assignee: dougt → nobody

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.