234725 - Make nsQueryInterface not be an nsCOMPtr_helper

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Right now whenever we do |do_QueryInterface| we use the nsCOMPtr_helper
mechanism to get the value into the nsCOMPtr.

However since we use |do_QueryInterface| so much it might be worth adding an
extra codepath in nsCOMPtr just for nsQueryInterface. This would reduce the
codesize at each caller, since there is no need to set a vtable, as well as save
a few cycles since the virtual call will now just be a normal call. In most
cases we could also get rid of the |nsresult*| member of nsQueryInterface since
it's hardly ever used.

I also want to look into inlineing nsQueryInterface::operator()

I have a patch that i'll attach once it's fully tested and i have some more
complete numbers on codesize savings. (initial testing showed 2k saved in
libxpcom.so)

Comment 1

[Alec Flett]

I'll bet codesize changes are pretty signifigant in other libraries, since much
of the xpcom library itself is implementing interfaces, not using (i.e.
do_QueryInterfac'ing them)

Comment 2

[Boris Zbarsky [:bzbarsky]]

According to grep, there are 7900 or so instances of do_QueryInterface in the
mozilla code, about 2000 of them in gklayout.  228 are in xpcom.  So if the
pattern holds, we should expect somewhere around 18k savings in gklayout and 70k
savings total.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Good estimate, i got 103k savings on linux with gcc 3.3.2. However i realized
that i can squeeze this a bit further so i'm trying that out now

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v1

The final savings ended up at 170k on linux with gcc 3.3.2.

I still need to try this on more platforms, but i don't see a reason why it
shouldn't be an improvment across the board.

I'm not exited about having special mechanisms both for nsQueryInterface and
nsQueryInterfaceWithError, but a large part of the saving comes from only
having one 4byte member in nsQueryInterface. I could make
nsQueryInterfaceWithError use the "old" nsCOMPtr_helper mecansim, but it feels
bad to make |do_QueryInterface(foo)| and |do_QueryInterface(foo, &rv)| be so
different.

I didn't inline nsQueryInterface::operator() yet since that might increase
filesize and the benefit is debatable. I'll investigate in a separate bug.

Comment 5

[Alec Flett]

I think making them different to avoid the extra nsresult member is genius,
myself :)

Its like the stupid bullet pointer member in nsBlockFrame.. we have enough block
frames that I'll bet those 4 bytes are taking up a nice little piece of heap.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 141711 [details] [diff] [review]
Patch v1

Tried the patch on windows and it looks like similar improvements. Works fine
on mac too though i don't have any numbers yet there.

I feel confident enough that this is a good thing so requesting reviews.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

just got the mac numbers, they're slightly better then on linux even!

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 141711 [details] [diff] [review]
Patch v1

Btw, this is the code that is generated on windows. 

Before:
00000006: 8B 45 08	     mov	 eax,dword ptr [ebp+8]
00000009: 83 65 FC 00	     and	 dword ptr [ebp-4],0
0000000D: 83 65 08 00	     and	 dword ptr [ebp+8],0
00000011: 89 45 F8	     mov	 dword ptr [ebp-8],eax
00000014: 8D 45 F4	     lea	 eax,[ebp-0Ch]
00000017: 68 00 00 00 00     push	 offset ?TestQI@@YAXPAVnsIAtom@@@Z  
<-- iid
0000001C: 50		     push	 eax
0000001D: 8D 4D 08	     lea	 ecx,[ebp+8]
00000020: C7 45 F4 00 00 00 00 mov	 dword ptr [ebp-0Ch],offset
?TestQI@@YAXPAVnsIAtom@@@Z <-- nsQueryInterface vtable
00000027: E8 00 00 00 00     call	 0000002C   <--
nsCOMPtr_base::assign_from_helper

After:
00000004: 83 65 FC 00	     and	 dword ptr [ebp-4],0
00000008: 68 00 00 00 00     push	 offset ?TestQI@@YAXPAVnsIAtom@@@Z  
<-- iid
0000000D: FF 75 08	     push	 dword ptr [ebp+8]
00000010: 8D 4D FC	     lea	 ecx,[ebp-4]
00000013: E8 00 00 00 00     call	 00000018   <--
nsCOMPtr_base::assign_from_helper


The improvement is more or less exactly the same as on linux. The main
difference is in parameter passing to nsCOMPtr_base::assign_from_helper and the
code to get the vtable. The object to QI is located on [ebp+8] in both
snippets.

Comment 9

[Bernard Alleysson]

This looks very promising (170k of code saved !)
What's the status on this bug ? 
Is there any binary compatibility problem with this patch ?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, there is no binary compatibility issues with this patch. If you look at the
attachmentlist you'll see that the status is that the patch is done and awaits
reviews.

Comment 11

[Alec Flett]

Comment on attachment 141711 [details] [diff] [review]
Patch v1

sr=alecf

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 141711 [details] [diff] [review]
Patch v1

Changing review-request to try to get this into 1.7b since dbaron is away

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That sounded wrong, i'm not trying to get this into 1.7b since dbaron's away,
i'm changing the request because of it :-)

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 141711 [details] [diff] [review]
Patch v1

>Index: xpcom/glue/nsCOMPtr.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/glue/nsCOMPtr.h,v
>retrieving revision 1.113
>diff -u -6 -p -r1.113 nsCOMPtr.h
>--- xpcom/glue/nsCOMPtr.h	13 Nov 2003 07:37:14 -0000	1.113
>+++ xpcom/glue/nsCOMPtr.h	19 Feb 2004 02:47:56 -0000
>@@ -324,41 +324,70 @@ class nsCOMPtr_helper
>         - (except for its name) |operator()| is a valid [XP]COM `getter'
>         - the interface pointer that it returns is already |AddRef()|ed (as from any good getter)
>         - it matches the type requested with the supplied |nsIID| argument
>         - its constructor provides an optional |nsresult*| that |operator()| can fill
>           in with an error when it is executed
>           
>-      See |class nsQueryInterface| for an example.
>+      See |class nsGetInterface| for an example.
>     */
>   {
>     public:
>       virtual nsresult operator()( const nsIID&, void** ) const = 0;
>   };
> 
>-class NS_COM nsQueryInterface : public nsCOMPtr_helper
>+/*
>+  |nsQueryInterface| could have been implemented as an |nsCOMPtr_helper| to
>+  avoid adding specialized machinery in |nsCOMPtr|, But |do_QueryInterface|
>+  is called often enough that the codesize savings are big enough to
>+  warrent the specialcasing.

"warrant"

>+*/
>+
>+class NS_COM nsQueryInterface
>+  {
>+    public:
>+      nsQueryInterface( nsISupports* aRawPtr )
>+          : mRawPtr(aRawPtr)
>+        {
>+          // nothing else to do here
>+        }
>+
>+      nsresult operator()( const nsIID& aIID, void** ) const;
>+
>+    private:
>+      nsISupports*  mRawPtr;
>+  };
>+
>+class NS_COM nsQueryInterfaceWithError
>   {
>     public:
>-      nsQueryInterface( nsISupports* aRawPtr, nsresult* error )
>+      nsQueryInterfaceWithError( nsISupports* aRawPtr, nsresult* error )
>           : mRawPtr(aRawPtr),
>             mErrorPtr(error)
>         {
>           // nothing else to do here
>         }
> 
>-      virtual nsresult operator()( const nsIID& aIID, void** ) const;
>+      nsresult operator()( const nsIID& aIID, void** ) const;
> 
>     private:
>       nsISupports*  mRawPtr;
>       nsresult*     mErrorPtr;
>   };
> 
> inline
>-const nsQueryInterface
>-do_QueryInterface( nsISupports* aRawPtr, nsresult* error = 0 )
>+nsQueryInterface
>+do_QueryInterface( nsISupports* aRawPtr )
>+  {
>+    return nsQueryInterface(aRawPtr);
>+  }
>+
>+inline
>+nsQueryInterfaceWithError
>+do_QueryInterface( nsISupports* aRawPtr, nsresult* error )
>   {
>-    return nsQueryInterface(aRawPtr, error);
>+    return nsQueryInterfaceWithError(aRawPtr, error);
>   }
> 
> template <class T>
> inline
> void
> do_QueryInterface( already_AddRefed<T>& )
>@@ -402,12 +431,14 @@ class nsCOMPtr_base
>           // nothing else to do here
>         }
> 
>       NS_COM ~nsCOMPtr_base();
> 
>       NS_COM void    assign_with_AddRef( nsISupports* );
>+      NS_COM void    assign_from_qi( const nsQueryInterface, const nsIID& );
>+      NS_COM void    assign_from_qi_with_error( const nsQueryInterfaceWithError&, const nsIID& );
>       NS_COM void    assign_from_helper( const nsCOMPtr_helper&, const nsIID& );
>       NS_COM void**  begin_assignment();
> 
>     protected:
>       NS_MAY_ALIAS_PTR(nsISupports) mRawPtr;
> 
>@@ -444,12 +475,14 @@ class nsCOMPtr
>   #define NSCAP_CTOR_BASE(x) nsCOMPtr_base(x)
> #else
>   #define NSCAP_CTOR_BASE(x) mRawPtr(x)
> 
>     private:
>       void    assign_with_AddRef( nsISupports* );
>+      void    assign_from_qi( const nsQueryInterface, const nsIID& );
>+      void    assign_from_qi_with_error( const nsQueryInterfaceWithError&, const nsIID& );
>       void    assign_from_helper( const nsCOMPtr_helper&, const nsIID& );
>       void**  begin_assignment();
> 
>       void
>       assign_assuming_AddRef( T* newPtr )
>         {
>@@ -527,33 +560,37 @@ class nsCOMPtr
>           // construct from |dont_AddRef(expr)|
>         {
>           NSCAP_LOG_ASSIGNMENT(this, aSmartPtr.mRawPtr);
>           NSCAP_ASSERT_NO_QUERY_NEEDED();
>         }
> 
>-      nsCOMPtr( const nsCOMPtr_helper& helper )
>+      nsCOMPtr( const nsQueryInterface qi )
>             : NSCAP_CTOR_BASE(0)
>-          // ...and finally, anything else we might need to construct from
>-          //  can exploit the |nsCOMPtr_helper| facility
>+          // construct from |do_QueryInterface(expr)|
>         {
>           NSCAP_LOG_ASSIGNMENT(this, 0);
>-          assign_from_helper(helper, NS_GET_IID(T));
>-          NSCAP_ASSERT_NO_QUERY_NEEDED();
>+          assign_from_qi(qi, NS_GET_IID(T));
>         }
> 
>-#ifdef NSCAP_FEATURE_TEST_DONTQUERY_CASES
>-        // For debug only --- this particular helper doesn't need to do the
>-        //  |NSCAP_ASSERT_NO_QUERY_NEEDED()| test.  In fact, with the logging
>-        //  changes, skipping the query test prevents infinite recursion.
>-      nsCOMPtr( const nsQueryInterface& helper )
>+      nsCOMPtr( const nsQueryInterfaceWithError& qi )
>+            : NSCAP_CTOR_BASE(0)
>+          // construct from |do_QueryInterface(expr, &rv)|
>+        {
>+          NSCAP_LOG_ASSIGNMENT(this, 0);
>+          assign_from_qi_with_error(qi, NS_GET_IID(T));
>+        }
>+
>+      nsCOMPtr( const nsCOMPtr_helper& helper )
>             : NSCAP_CTOR_BASE(0)
>+          // ...and finally, anything else we might need to construct from
>+          //  can exploit the |nsCOMPtr_helper| facility
>         {
>           NSCAP_LOG_ASSIGNMENT(this, 0);
>           assign_from_helper(helper, NS_GET_IID(T));
>+          NSCAP_ASSERT_NO_QUERY_NEEDED();
>         }
>-#endif
> 
> 
>         // Assignment operators
> 
>       nsCOMPtr<T>&
>       operator=( const nsCOMPtr<T>& rhs )
>@@ -579,32 +616,36 @@ class nsCOMPtr
>           assign_assuming_AddRef(rhs.mRawPtr);
>           NSCAP_ASSERT_NO_QUERY_NEEDED();
>           return *this;
>         }
> 
>       nsCOMPtr<T>&
>-      operator=( const nsCOMPtr_helper& rhs )
>-          // ...and finally, anything else we might need to assign from
>-          //  can exploit the |nsCOMPtr_helper| facility.
>+      operator=( const nsQueryInterface rhs )
>+          // assign from |do_QueryInterface(expr)|
>         {
>-          assign_from_helper(rhs, NS_GET_IID(T));
>-          NSCAP_ASSERT_NO_QUERY_NEEDED();
>+          assign_from_qi(rhs, NS_GET_IID(T));
>+          return *this;
>+        }
>+
>+      nsCOMPtr<T>&
>+      operator=( const nsQueryInterfaceWithError& rhs )
>+          // assign from |do_QueryInterface(expr, &rv)|
>+        {
>+          assign_from_qi_with_error(rhs, NS_GET_IID(T));
>           return *this;
>         }
> 
>-#ifdef NSCAP_FEATURE_TEST_DONTQUERY_CASES
>-        // For debug only --- this particular helper doesn't need to do the
>-        //  |NSCAP_ASSERT_NO_QUERY_NEEDED()| test.  In fact, with the logging
>-        //  changes, skipping the query test prevents infinite recursion.
>       nsCOMPtr<T>&
>-      operator=( const nsQueryInterface& rhs )
>+      operator=( const nsCOMPtr_helper& rhs )
>+          // ...and finally, anything else we might need to assign from
>+          //  can exploit the |nsCOMPtr_helper| facility.
>         {
>           assign_from_helper(rhs, NS_GET_IID(T));
>+          NSCAP_ASSERT_NO_QUERY_NEEDED();
>           return *this;
>         }
>-#endif
> 
>       void
>       swap( nsCOMPtr<T>& rhs )
>           // ...exchange ownership with |rhs|; can save a pair of refcount operations
>         {
> #ifdef NSCAP_FEATURE_USE_BASE
>@@ -776,12 +817,28 @@ class nsCOMPtr<nsISupports>
>             : nsCOMPtr_base(aSmartPtr.mRawPtr)
>           // construct from |dont_AddRef(expr)|
>         {
>           NSCAP_LOG_ASSIGNMENT(this, aSmartPtr.mRawPtr);
>         }
> 
>+      nsCOMPtr( const nsQueryInterface qi )
>+            : nsCOMPtr_base(0)
>+          // assign from |do_QueryInterface(expr)|
>+        {
>+          NSCAP_LOG_ASSIGNMENT(this, 0);
>+          assign_from_qi(qi, NS_GET_IID(nsISupports));
>+        }
>+
>+      nsCOMPtr( const nsQueryInterfaceWithError& qi )
>+            : nsCOMPtr_base(0)
>+          // assign from |do_QueryInterface(expr, &rv)|
>+        {
>+          NSCAP_LOG_ASSIGNMENT(this, 0);
>+          assign_from_qi_with_error(qi, NS_GET_IID(nsISupports));
>+        }
>+
>       nsCOMPtr( const nsCOMPtr_helper& helper )
>             : nsCOMPtr_base(0)
>           // ...and finally, anything else we might need to construct from
>           //  can exploit the |nsCOMPtr_helper| facility
>         {
>           NSCAP_LOG_ASSIGNMENT(this, 0);
>@@ -813,12 +870,28 @@ class nsCOMPtr<nsISupports>
>         {
>           assign_assuming_AddRef(rhs.mRawPtr);
>           return *this;
>         }
> 
>       nsCOMPtr<nsISupports>&
>+      operator=( const nsQueryInterface rhs )
>+          // assign from |do_QueryInterface(expr)|
>+        {
>+          assign_from_qi(rhs, NS_GET_IID(nsISupports));
>+          return *this;
>+        }
>+
>+      nsCOMPtr<nsISupports>&
>+      operator=( const nsQueryInterfaceWithError& rhs )
>+          // assign from |do_QueryInterface(expr, &rv)|
>+        {
>+          assign_from_qi_with_error(rhs, NS_GET_IID(nsISupports));
>+          return *this;
>+        }
>+
>+      nsCOMPtr<nsISupports>&
>       operator=( const nsCOMPtr_helper& rhs )
>           // ...and finally, anything else we might need to assign from
>           //  can exploit the |nsCOMPtr_helper| facility.
>         {
>           assign_from_helper(rhs, NS_GET_IID(nsISupports));
>           return *this;
>@@ -944,12 +1017,32 @@ template <class T>
> void
> nsCOMPtr<T>::assign_with_AddRef( nsISupports* rawPtr )
>   {
>     if ( rawPtr )
>       NSCAP_ADDREF(this, rawPtr);
>     assign_assuming_AddRef(NS_REINTERPRET_CAST(T*, rawPtr));
>+  }
>+
>+template <class T>
>+void
>+nsCOMPtr<T>::assign_from_qi( const nsQueryInterface qi, const nsIID& aIID )
>+  {
>+    T* newRawPtr;
>+    if ( NS_FAILED( qi(aIID, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) )
>+      newRawPtr = 0;
>+    assign_assuming_AddRef(newRawPtr);
>+  }
>+
>+template <class T>
>+void
>+nsCOMPtr<T>::assign_from_qi_with_error( const nsQueryInterfaceWithError& qi, const nsIID& aIID )
>+  {
>+    T* newRawPtr;
>+    if ( NS_FAILED( qi(aIID, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) )
>+      newRawPtr = 0;
>+    assign_assuming_AddRef(newRawPtr);
>   }
> 
> template <class T>
> void
> nsCOMPtr<T>::assign_from_helper( const nsCOMPtr_helper& helper, const nsIID& aIID )
>   {

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in. Savings ranged from 64k (windows) to 340k (linux)

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

no noticable affect on any performance numbers though, which was expected.

Comment 17

[Philip K. Warren]

See Bug 239831 for one side effect of this checkin.