264456 - do_GetService causes unnecessary bloat

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

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

do_GetService causes unnecessary code bloat due to having an nsCOMPtr member on
nsGetServiceBy*, and doing a do_QueryInterface to initialize that member.

Comment 1

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

Attachment: patch

Comment 2

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

Attachment: patch

Comment 3

[Bernard Alleysson]

unrelated, but please note that do_GetInterface also has unnecessary bloat:
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsIInterfaceRequestorUtils.h#71

nsGetInterface::operator() can be modified to use raw mSource XPCOM pointer
It would also save one implicit AddRef()/Release() call for every use of
do_GetInterface

Comment 4

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

Attachment: patch for nsIInterfaceRequestorUtils

Comment 5

[Darin Fisher]

Comment on attachment 162148 [details] [diff] [review]
patch

r=darin

Comment 6

[Darin Fisher]

Comment on attachment 162203 [details] [diff] [review]
patch for nsIInterfaceRequestorUtils

r=darin

Comment 7

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

Fixes checked in to trunk, 2004-10-15 10:44/11:03 -0700.

Comment 8

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

These together seem to have improved Tp by about 1.5% - 2% on btek.

Comment 9

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

From luna (Suite Linux), for both patches:

Z:17.93MB
Zdiff:-202816 (+859/-203675)
mZ:11.27MB
mZdiff:-95724 (+223/-95947)

From imola (Firefox Mac), for the first patch only, but with some of jst's
changes as well:

Z:15.53MB
Zdiff:-82096 (+34644/-116740)

Comment 10

[Bernard Alleysson]

I think that second patch is more about performance (less refcounting). I think
that most compilers will inline the nsGetInterface constructor after the second
patch was checked in (I don't think it was inlined before because of the
nsCOMPtr<> constructor, at least at -O1). So overall it is not evident that
there's some code reduction with the second patch.

I have another patch (almost ready) that does remove do_GetInterface() code
bloat at all call sites by removing the inline error pointer assignment in
nsGetInterface ctor. I think I'll file a new bug about that.

Comment 11

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

From egg (minimo tinderbox):
  first patch:
    mZ:7.506MB
    mZdiff:-51196 (+177/-51373)
  second patch:
    mZ:7.500MB
    mZdiff:-6840 (+2/-6842)

Comment 12

[Darin Fisher]

great stuff!

Comment 13

[[e-mail]]

Comment on attachment 162148 [details] [diff] [review]
patch

>+    nsCOMPtr<nsIServiceManager> serviceManager =
>+        do_QueryInterface(mServiceManager);
>+    if ( serviceManager ) {
>+        status = serviceManager->GetService(mCID, aIID, (void**)aInstancePtr);
>     } else {
>         nsCOMPtr<nsIServiceManager> mgr;
>         NS_GetServiceManager(getter_AddRefs(mgr));
>         if (mgr)
>             status = mgr->GetService(mCID, aIID, (void**)aInstancePtr);
>     }
Is the compiler is smart enough to optimize this as follows?
nsCOMPtr<nsIServiceManager> mgr(do_QueryInterface(mServiceManager));
if (!mgr)
  NS_GetServiceManager(getter_AddRefs(mgr));
if (mgr)
  status = mgr->GetService(mCID, aIID, (void**)aInstancePtr);

Comment 14

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

Perhaps.  But what's really meant is:

nsCOMPtr<nsIServiceManager> mgr = do_QueryInterface(mServiceManager);
if (mgr || (NS_GetServiceManager(getter_AddRefs(mgr)), mgr))
  status = mgr->GetService(mCID, aIID, (void**)aInstancePtr);

Comment 15

[Brian Ryner (not reading)]

Attachment: remove nsIServiceManager parameter

We never actually pass a non-null service manager to do_GetService().

Comment 16

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

Comment on attachment 162345 [details] [diff] [review]
remove nsIServiceManager parameter

>-    nsCOMPtr<nsICategoryManager> catman =
>-        do_GetService(kCategoryManagerCID, &rv);
>-    if (NS_FAILED(rv)) goto error;
>+    nsCOMPtr<nsICategoryManager> catman;

This is wrong.

Comment 17

[Brian Ryner (not reading)]

(In reply to comment #16)
> (From update of attachment 162345 [details] [diff] [review])
> >-    nsCOMPtr<nsICategoryManager> catman =
> >-        do_GetService(kCategoryManagerCID, &rv);
> >-    if (NS_FAILED(rv)) goto error;
> >+    nsCOMPtr<nsICategoryManager> catman;
> 
> This is wrong.
> 

What, exactly?

Comment 18

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

Comment on attachment 162345 [details] [diff] [review]
remove nsIServiceManager parameter

Oh, I see.  But still, having to write NS_GET_IID() with an argument that's
something other than a template parameter is evil.

Comment 19

[Brian Ryner (not reading)]

(In reply to comment #18)
> (From update of attachment 162345 [details] [diff] [review])
> Oh, I see.  But still, having to write NS_GET_IID() with an argument that's
> something other than a template parameter is evil.
> 

Are you referring to using it in conjunction with getter_AddRefs() ?  If so, I'm
ok with making catman a raw pointer there, though I don't see what it gains us.

Comment 20

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

No, I'm referring to having to type "nsICategoryManager" twice for the same
variable rather than using do_GetService or CallGetService.  If you think
reusing the service manager pointer rather than calling NS_GetServiceManager
twice is that useful here, then you probably shouldn't remove the
functionality... :-)

Comment 21

[Bernard Alleysson]

Now it is clear that do_GetService() relies heavily on NS_GetServiceManager()
http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsComponentManager.cpp#3722

Could it be possible to add a special NS_GetServiceManager function that doesn't
addref ? It would save a lot of AddRef/Release calls. There's more than 3000
occurrences of do_GetService().

Comment 22

[Brian Ryner (not reading)]

I'm not just removing the functionality for the fun of it; passing the extra
parameter to nsGetServiceBy* makes extra work for the caller, and most callers
aren't able to reuse the service manager because they're only interested in a
single service.

Here inside the do_GetServiceFromCategory() implementation, I think it makes
sense to be as efficient as we can be, even if it means typing
nsICategoryManager twice.  It's not going to be a big deal either way though,
do_GetServiceFromCategory has exactly one caller, and it's not all that
performance sensitive.

Comment 23

[Brian Ryner (not reading)]

(In reply to comment #21)
> Could it be possible to add a special NS_GetServiceManager function that doesn't
> addref ? It would save a lot of AddRef/Release calls. There's more than 3000
> occurrences of do_GetService().

I thought about that as well.. seems like a good idea.

Comment 24

[Bernard Alleysson]

After the removal of mServiceManager, it is now possible to consider
specializing nsGetServiceXX for the case where mErrorPtr is null, I mean doing
what has been done with nsQueryInterface/nsQueryInterfaceWithError(but
operator() would still be virtual here):
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsCOMPtr.h#359

It would remove another extra member for some (most ?) callers, simplify the
inline constructor, possibly remove some more bloat. (3000 do_GetService is less
than 8000 do_QueryInterface but still...)

Comment 25

[Brian Ryner (not reading)]

Comment on attachment 162345 [details] [diff] [review]
remove nsIServiceManager parameter

So can we go ahead and do this? iirc this saved about 30KB code size on Linux.

Comment 26

[Darin Fisher]

bryner: you might want to take a look at my patch in bug 263360.  we might want
to use the same approach for both of these bugs.

Comment 27

[Darin Fisher]

Comment on attachment 162345 [details] [diff] [review]
remove nsIServiceManager parameter

This needs to be revised now that the patch for bug 239088 has landed. 
Removing from my review queue.

Comment 28

[Darin Fisher]

grr.. copy/paste error.  i meant bug 263360 ;-)

Comment 29

[Brian Ryner (not reading)]

reopening for new patch.

Comment 30

[Brian Ryner (not reading)]

Attachment: revised patch

Comment 31

[Bernard Alleysson]

I like it !

Some remarks:

1)
all "operator()" in nscomptr.h could be simplified by changing the signature
from
nsresult NS_FASTCALL operator()( const nsIID&, void** ) const;
to
void* NS_FASTCALL operator()( const nsIID& ) const;

assuming that the implementations return NULL when an error occurs

then (for instance)

void
+nsCOMPtr_base::assign_from_gs_cid( const nsGetServiceByCID gs, const nsIID& iid )
+	{
+		nsISupports* newRawPtr;
+		if ( NS_FAILED( gs(iid, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) )
+			newRawPtr = 0;
+    assign_assuming_AddRef(newRawPtr);
+	}

would be

void
+nsCOMPtr_base::assign_from_gs_cid( const nsGetServiceByCID gs, const nsIID& iid )
+	{
+		nsISupports* newRawPtr;
+		newRawPtr = gs(iid);
+    assign_assuming_AddRef(newRawPtr);
+	}

or simply

void
+nsCOMPtr_base::assign_from_gs_cid( const nsGetServiceByCID gs, const nsIID& iid )
+	{
+    assign_assuming_AddRef(gs(iid));
+	}

it will result in less call parameters overhead (push/pop, etc...)

2)
the same "WithError" trick could be used with nsGetInterface
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsIInterfaceRequestorUtils.h#58
but it is probably another bug though it could be done easily
There are about 1000 do_GetInterface() calls iirc

3)
+      void    assign_from_gs_cid( const nsGetServiceByCID, const nsIID& );
+      void    assign_from_gs_cid_with_error( const nsGetServiceByCIDWithError&,
const nsIID& );
+      void    assign_from_gs_contractid( const nsGetServiceByContractID, const
nsIID& );
+      void    assign_from_gs_contractid_with_error( const
nsGetServiceByContractIDWithError&, const nsIID& );

they all could be called "assigned_from_helper" because the parameters type are
different : no need to find new names and it is shorter to write

4)
it looks like bug that bug 264710 is fixed by this patch too.

Comment 32

[Darin Fisher]

Comment on attachment 166953 [details] [diff] [review]
revised patch

>Index: xpcom/components/nsComponentManager.cpp

>+    rv = compMgr->nsComponentManagerImpl::GetService(kCategoryManagerCID,
>+                                                NS_GET_IID(nsICategoryManager),
>+                                                     getter_AddRefs(catman));

nit: indentation


r=darin

Comment 33

[Brian Ryner (not reading)]

> 1)
> all "operator()" in nscomptr.h could be simplified by changing the signature
> from
> nsresult NS_FASTCALL operator()( const nsIID&, void** ) const;
> to
> void* NS_FASTCALL operator()( const nsIID& ) const;
> 
> assuming that the implementations return NULL when an error occurs

Let's hold off on that; I don't think we can say for sure that NULL will be
returned when an error occurs.

> 2)
> the same "WithError" trick could be used with nsGetInterface
> http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsIInterfaceRequestorUtils.h#58
> but it is probably another bug though it could be done easily
> There are about 1000 do_GetInterface() calls iirc

Sure, separate bug.

> 3)
> +      void    assign_from_gs_cid( const nsGetServiceByCID, const nsIID& );
> +      void    assign_from_gs_cid_with_error( const nsGetServiceByCIDWithError&,
> const nsIID& );
> +      void    assign_from_gs_contractid( const nsGetServiceByContractID, const
> nsIID& );
> +      void    assign_from_gs_contractid_with_error( const
> nsGetServiceByContractIDWithError&, const nsIID& );
> 
> they all could be called "assigned_from_helper" because the parameters type are
> different : no need to find new names and it is shorter to write

True, but I actually like it being explicit so it's clear whether the error
parameter is present.

Comment 34

[Brian Ryner (not reading)]

checked in.

Comment 35

[Bernard Alleysson]

I've submitted several bugs with patches to consider applying the same tricks
with do_GetInterface (bug 271845), do_GetIOService (bug 271823),
do_GetFastLoadService (bug 271824), do_CreateInstance (bug 271817)