271845 - do_GetInterface causes unnecessary bloat

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Bernard Alleysson]

Use nsGetInterfaceWithError/nsGetInterface to avoid extra member mErrorPtr when
it is not needed

Comment 1

[Bernard Alleysson]

Attachment: patch 1

nsGetInterface/nsGetInterfaceWithError, make nsGetInterface* operator() not
virtual, and remove overhead of nsCOMPtr<> from nsGetInterface* operator()

Comment 2

[Bernard Alleysson]

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

Brian, would you review the patch ?
It's the same as bug 264456
(nsGetServiceBy*WithError)

Comment 3

[Brian Ryner (not reading)]

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

looks ok, i'd like to see some codesize numbers first though

Comment 4

[Bernard Alleysson]

I've just compiled Mozilla Suite on Windows with VC7 and there's no codesize
change. A quick examination shows that gklayout.dll is 1Kb smaller while
xpcom_core.dll is 1Kb bigger but overall the size of all DLLs is 12830208 with
or without the patch.

VC generates better code than GCC and there could be a small codesize win on Linux.

This patch is a small perf win because it makes operator() not virtual, it
removes nsCOMPtr<> usage from operator() and because the generated code is
slightly better at all call sites for do_GetInterface(ptr) variant.
I think that callers expect do_GetInterface to be as "fast" as possible and this
patch can help.

This patch also allows nsGetInterface operator() to be removed and inlined at
its call sites (only 2 call sites, in nsCOMPtr.h, nsCOMPtr.cpp !) so there's
more to come ...

Comment 5

[Bernard Alleysson]

Attachment: patch 2

It should be a small code size win on Linux.
Same as the previous patch but changed "if ( factoryPtr )" to "if
NS_SUCCEEDED(status)" in nsIInterfaceRequestorUtils.cpp, for correctness.

Comment 6

[Darin Fisher]

Comment on attachment 168750 [details] [diff] [review]
patch 2

can you post codesize reduction numbers?  i'd like to know how this impacts the
size of mozilla as well as the size of the xpcom_core library.

Comment 7

[Bernard Alleysson]

Attachment: patch 3

Same as the previous patch, but inlined/removed nsGetInterface operator(), for
more performance, (nsIInterfaceRequestorUtils.cpp should be removed)

Comment 8

[Bernard Alleysson]

Attachment: codesize numbers on Windows

On Windows, the total saving is 2 kb and you have a more efficient
do_GetInterface() implementation (no more virtual call, no more nsCOMPtr<>
overhead, and inlined code).

I can't have code size numbers on Linux, sorry, all I can say is that it is
similar to what was done with nsQueryInterface. There are about 400
do_GetInterface(), most of them don't use the error variant. Looking at numbers
for nsQueryInterface, I think that it should save about 5Kb. And of course
you'll still get a faster do_GetInterface().

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 168959 [details] [diff] [review]
patch 3

My personal thought is that the numbers don't justify adding this complexity to
the code.  Darin, what do you think?

Comment 10

[Darin Fisher]

Comment on attachment 168959 [details] [diff] [review]
patch 3

doesn't seem worth it to me either.  sorry, i know this was a large patch, and you are probably unhappy to see it get rejected like this.  next time, i recommend doing more proof-of-concept work up front.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> (From update of attachment 168959 [details] [diff] [review] [edit])
> doesn't seem worth it to me either.  sorry, i know this was a large patch, and
> you are probably unhappy to see it get rejected like this.  next time, i
> recommend doing more proof-of-concept work up front.

Does that make this bug WONTFIX?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Marking WONTFIX per comments #9 and #10. Bernard, feel free to reopen if you object.