If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

do_GetService causes unnecessary bloat

RESOLVED FIXED in mozilla1.8alpha5

Status

()

Core
XPCOM
P2
normal
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({footprint})

Trunk
mozilla1.8alpha5
footprint
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

13 years ago
do_GetService causes unnecessary code bloat due to having an nsCOMPtr member on
nsGetServiceBy*, and doing a do_QueryInterface to initialize that member.
(Assignee)

Comment 1

13 years ago
Created attachment 162147 [details] [diff] [review]
patch
(Assignee)

Comment 2

13 years ago
Created attachment 162148 [details] [diff] [review]
patch
Attachment #162147 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8alpha5
(Assignee)

Updated

13 years ago
Attachment #162148 - Flags: review?(darin)

Comment 3

13 years ago
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
(Assignee)

Comment 4

13 years ago
Created attachment 162203 [details] [diff] [review]
patch for nsIInterfaceRequestorUtils

Comment 5

13 years ago
Comment on attachment 162148 [details] [diff] [review]
patch

r=darin
Attachment #162148 - Flags: review?(darin) → review+
(Assignee)

Updated

13 years ago
Attachment #162203 - Flags: review?(darin)

Comment 6

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

r=darin
Attachment #162203 - Flags: review?(darin) → review+
(Assignee)

Comment 7

13 years ago
Fixes checked in to trunk, 2004-10-15 10:44/11:03 -0700.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

13 years ago
These together seem to have improved Tp by about 1.5% - 2% on btek.
(Assignee)

Comment 9

13 years ago
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

13 years ago
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.
(Assignee)

Comment 11

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

Comment 12

13 years ago
great stuff!

Comment 13

13 years ago
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);
(Assignee)

Comment 14

13 years ago
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);
Created attachment 162345 [details] [diff] [review]
remove nsIServiceManager parameter

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

Comment 16

13 years ago
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.
Attachment #162345 - Flags: review-
(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?

(Assignee)

Comment 18

13 years ago
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.
Attachment #162345 - Flags: review-
(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.
(Assignee)

Comment 20

13 years ago
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

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

13 years ago
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 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.
Attachment #162345 - Flags: review?(darin)

Comment 26

13 years ago
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.

Updated

13 years ago
Blocks: 239088

Comment 27

13 years ago
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.
Attachment #162345 - Flags: review?(darin)

Comment 28

13 years ago
grr.. copy/paste error.  i meant bug 263360 ;-)
reopening for new patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 166953 [details] [diff] [review]
revised patch
Attachment #166953 - Flags: review?(darin)

Comment 31

13 years ago
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

13 years ago
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
Attachment #166953 - Flags: review?(darin) → review+
> 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.
checked in.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 35

13 years ago
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)

Updated

13 years ago
Keywords: footprint
You need to log in before you can comment on or make changes to this bug.