Closed
Bug 271817
Opened 20 years ago
Closed 11 years ago
do_CreateInstance causes unnecessary bloat
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: bernard.alleysson, Unassigned)
Details
Attachments
(2 files)
10.70 KB,
patch
|
bryner
:
review+
|
Details | Diff | Splinter Review |
3.63 KB,
text/plain
|
Details |
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•20 years ago
|
||
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•20 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 3•20 years ago
|
||
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•20 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•20 years ago
|
||
Sorry for the spam, just to make sure you receive bugmail (see esp my previous comment on codesize)
Reporter | ||
Comment 6•20 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•20 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•20 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•20 years ago
|
||
xpcom_core.dll is 3 kb bigger, but other dlls are smaller, and overall there's 1 kb code size increase
Comment 10•18 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•18 years ago
|
||
was there any perf. gain?
Updated•18 years ago
|
QA Contact: xpcom
Updated•18 years ago
|
Assignee: dougt → nobody
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•