Closed Bug 234725 Opened 21 years ago Closed 21 years ago

Make nsQueryInterface not be an nsCOMPtr_helper

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

Details

Attachments

(1 file)

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)
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)
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.
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
Status: NEW → ASSIGNED
Attached patch Patch v1Splinter Review
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.
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 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.
Attachment #141711 - Flags: superreview?(alecf)
Attachment #141711 - Flags: review?(dbaron)
just got the mac numbers, they're slightly better then on linux even!
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.
This looks very promising (170k of code saved !) What's the status on this bug ? Is there any binary compatibility problem with this patch ?
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 on attachment 141711 [details] [diff] [review] Patch v1 sr=alecf
Attachment #141711 - Flags: superreview?(alecf) → superreview+
Comment on attachment 141711 [details] [diff] [review] Patch v1 Changing review-request to try to get this into 1.7b since dbaron is away
Attachment #141711 - Flags: review?(dbaron) → review?(bryner)
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 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 ) > {
Attachment #141711 - Flags: review?(bryner) → review+
Checked in. Savings ranged from 64k (windows) to 340k (linux)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
no noticable affect on any performance numbers though, which was expected.
See Bug 239831 for one side effect of this checkin.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: