Last Comment Bug 391275 - Remove the QueryInterface outparam semantics (change its signature to void * QueryInterface(REFNSIID iid))
: Remove the QueryInterface outparam semantics (change its signature to void * ...
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on: 435431 662693 395444
Blocks: 657001
  Show dependency treegraph
 
Reported: 2007-08-07 14:03 PDT by (dormant account)
Modified: 2011-10-10 12:28 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first attempt (132.39 KB, patch)
2007-08-07 16:42 PDT, (dormant account)
no flags Details | Diff | Review
List of macros to rewrite (2.30 KB, text/plain)
2007-08-07 16:48 PDT, (dormant account)
no flags Details
Cleaner attempt (95.13 KB, patch)
2007-08-09 18:31 PDT, (dormant account)
no flags Details | Diff | Review
Manual rewriting, rev. 1 (47.92 KB, patch)
2007-08-13 13:16 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review
Modifications to help outparamdel process mozilla (3.47 KB, patch)
2007-08-14 11:39 PDT, (dormant account)
no flags Details | Diff | Review
Modifications to help outparamdel process mozilla (3.47 KB, patch)
2007-08-14 11:42 PDT, (dormant account)
no flags Details | Diff | Review
Modifications to help outparamdel process mozilla (3.47 KB, patch)
2007-08-14 11:44 PDT, (dormant account)
no flags Details | Diff | Review
Most complete patch yet (185.98 KB, patch)
2007-08-14 14:34 PDT, (dormant account)
no flags Details | Diff | Review
Full patch against mozilla-central (154.10 KB, patch)
2007-08-15 11:39 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Review
Modifications to help outparamdel process mozilla (3.72 KB, patch)
2007-08-22 16:56 PDT, (dormant account)
no flags Details | Diff | Review
Most complete patch yet (170.83 KB, patch)
2007-08-22 17:01 PDT, (dormant account)
no flags Details | Diff | Review
Manual rewriting (22.87 KB, patch)
2007-08-22 17:14 PDT, (dormant account)
no flags Details | Diff | Review
patch #2: Autogenerated qi rewrite (151.84 KB, patch)
2007-08-30 14:33 PDT, (dormant account)
no flags Details | Diff | Review
patch #3: Template fixup (746 bytes, patch)
2007-08-30 14:34 PDT, (dormant account)
no flags Details | Diff | Review
patch #4: Workaround for mcpp macro-annotation bug. This will be fixed soon. (639 bytes, patch)
2007-08-30 14:37 PDT, (dormant account)
no flags Details | Diff | Review
patch #5: Manual macro-rewriting (44.45 KB, patch)
2007-08-30 14:40 PDT, (dormant account)
no flags Details | Diff | Review
build config. had to disable a couple of things since they give mcpp trouble. (904 bytes, patch)
2007-08-30 15:05 PDT, (dormant account)
no flags Details | Diff | Review
New approach (292.14 KB, patch)
2011-09-19 20:50 PDT, :Ehsan Akhgari (busy, don't ask for review please)
benjamin: review-
Details | Diff | Review

Description (dormant account) 2007-08-07 14:03:33 PDT
Currently the signature of nsISupport::QueryInterface is

nsresult QueryInterface(REFNSIID iid, void **result);

I'd like it to be

void * QueryInterface(REFNSIID iid);

(return null on failure, instead of NS_ERROR_NO_INTERFACE)

The vast majority of queryinterface implementations are parameterized macros
that I can rewrite by hand relatively easily.

The vast majority of callers use do_QueryInterface or CallQueryInterface,
which can be rewritten by hand if desirable. The callers which pass &rv to
those funcs and then check NS_FAILED(rv) should use if (iface) instead.

It's the manual implementations and callers that I'd like to have help
rewriting. 

- --BDS
Comment 1 (dormant account) 2007-08-07 16:42:43 PDT
Created attachment 275694 [details] [diff] [review]
first attempt

This the first attempt, some things may not work right. I have to fix some dereferencing of void* casts.
Comment 2 (dormant account) 2007-08-07 16:48:48 PDT
Created attachment 275696 [details]
List of macros to rewrite

This is a list of macros. I can't rewrite these automatically. These will likely have to be done by hand.
Comment 3 Benjamin Smedberg [:bsmedberg] 2007-08-07 17:36:03 PDT
The macros that I can't rewrite by hand are
NS_FAILED
NS_SUCCEEDED
NS_IMETHOD
NS_IMETHODIMP
NS_ASSERTION

1) I'm interested to see what NS_IMETHOD and NS_IMETHODIMP are doing in the list to begin with
2) Usages such as 

if (NS_FAILED(foo->QueryInterface(NS_GET_IID(nsISomething), &bar))) really need to be rewritten to:
if (!(bar = foo->QueryInterface(NS_GET_IID(nsISomething))))
Comment 4 (dormant account) 2007-08-08 09:40:49 PDT
(In reply to comment #3)
> The macros that I can't rewrite by hand are
> NS_FAILED
> NS_SUCCEEDED
> NS_IMETHOD
> NS_IMETHODIMP
> NS_ASSERTION
> 
> 1) I'm interested to see what NS_IMETHOD and NS_IMETHODIMP are doing in the
> list to begin with

"I can't rewrite these automatically" should've said "most of these". There two macros obfuscate part of the function definition so I have to special-case rewrites involving them. I also need to handle all parameterless macros like constants.

Is there another macro to decorate functions returning values?

> 2) Usages such as 
> 
> if (NS_FAILED(foo->QueryInterface(NS_GET_IID(nsISomething), &bar))) really need
> to be rewritten to:
> if (!(bar = foo->QueryInterface(NS_GET_IID(nsISomething))))
> 

Thanks for the example. These have to be special-cased too. 
Comment 5 Benjamin Smedberg [:bsmedberg] 2007-08-09 11:51:01 PDT
Hey Taras, let's sit down at the onsite and discuss the limits/special cases for macro rewriting: I'm not sure I understand how it should work yet.

I suspect that outparam rewriting in general is going to need special-case for NS_FAILED/NS_SUCCEEDED because those patterns are so common.

And since basically every XPCOM method is decorated with NS_IMETHOD/NS_IMETHODIMP (virtual methods) or NS_METHOD (nonvirtual methods) those should probably be generally taken into account as well.
Comment 6 (dormant account) 2007-08-09 12:29:24 PDT
Don't worry I don't understand the ALL of the limitation either yet, that's why I'm working on this. It's a pretty hairy subject. 
Basic idea is that the C++ parser does not see the original source code, it sees a preprocessed version which looses all of the original positions and doesn't see preprocessor constructs like NS_FAILED, etc. This is a problem because I am not interested in patching the preprocessed output. For this reason I devised a set of preprocessor extensions that embed cpp-undo information into the preprocessed source code so it is possible to take preprocessed position and figure out the original position and whether the code is part of a macro. From there I can try to special case various mozilla macro idioms.
I'll be happy to discuss this more at the summit.

I just finished adding special cases for NS_SUCCEEDED and NS_FAILED. I also found a limitation my macro-understand extension to the preprocessor. I can't always get original source positions for macro parameters that contain macros in front of the thing I'm looking for.

So I can't do (NS_FAILED(foo->QueryInterface(NS_GET_IID(nsISomething), &bar))) unless NS_GET_IID is turned into a non-macro(ie inline func) or I expand the macro-expanding spec in MCPP. I'm working with the MCPP author to see if we can address this. Basically, it's annoying, but it's a not a show-stopper since turning macros into function is a sensible work-around in cases like these.

Now I need to add special-cases for NS_METHOD, NS_METHODIMP and I'll post another patch for you to comment on.

My goal is to have something that compiles & links by the end of the onsite. If I'm really lucky, I'll get most of it done by the end of this week.
Comment 7 (dormant account) 2007-08-09 18:31:33 PDT
Created attachment 276056 [details] [diff] [review]
Cleaner attempt

Here is another preview. I added support for NS_IMETHOD[IMP] and NS_FAILED & NS_SUCCEEDED, added casts from void (type*)QueryInterface, etc.

Still a lot of work left to be done for handling every type of queryinterface call. Ie getter_addrefs & other cases.
Comment 8 (dormant account) 2007-08-10 13:50:56 PDT
Given code like:

nsCOMPtr<nsILineIteratorNavigator> it; 
aBlockFrame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator),getter_AddRefs(it));

Is it correct to rewrite it as 
it = (nsIlineInteratorNavigator*) aBlockFrame->QueryInterface(NS_GET_IID(nsILineIteratorNavigator));

or should there be an already_AddRefed somewhere in there (maybe in the nsISupport signature).
Comment 9 Benjamin Smedberg [:bsmedberg] 2007-08-13 07:54:07 PDT
Hrm, I don't think there's a good way to put already_AddRefed in the signature, because it's untyped... the best you could do is already_AddRefed<nsISomeInterface> (nsISomeInterface is a typedef for nsISupports that means "any interface"), but that will cause problems with nsCOMPtr and friends, I think.

This code should have been written originally as:

nsCOMPtr<nsILineIteratorNavigator> it = do_QueryInterface(aBlockFrame);

If it's possible to automatically rewrite it to that, or at least to:
nsCOMPtr<nsILineIteratorNavigator> it;
it = do_QueryInterface(aBlockFrame);

I can hand-rewrite the do_QueryInterface goop.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-08-13 09:11:28 PDT
Do we depend on this being binary-compatible with Microsoft COM?  I thought there were some places in the Windows widget code that do, and I thought it was part of doing it this way originally.

I don't think people should ever write IIDs by hand -- we should make them use the C++ template system to link the C++ type to the IID.  Code should never say NS_GET_IID(...) with the argument being the name of an interface rather than a template parameter.  So I think it would be fine, frankly, if QueryInterface returned some new already_AddRefed-like thing; I don't think it should be exposed to callers.

I'm not happy with void*, since it's leak-prone.  Then again, I think at least some compilers implement struct returns as out parameters, even for small structs, so there might not be much gain any other way.
Comment 11 Benjamin Smedberg [:bsmedberg] 2007-08-13 09:55:38 PDT
The only place I know we depend on MSCOM compatibility is the IDispatch scripting in xpconnect. For the moment, consider this a mozilla2 experiment: I'm trying to get basic perf numbers from this experiment, and I'm making the assumption that XPCOM isn't going to stay binary compatible at all in the future.
Comment 12 Benjamin Smedberg [:bsmedberg] 2007-08-13 13:16:23 PDT
Created attachment 276520 [details] [diff] [review]
Manual rewriting, rev. 1

This patch manually rewrites all of xpcom/ to a building state. It adds an inline version of the old version of nsISupports::QueryInterface so that most clients of the method will continue to work (obviously implementers need to be rewritten).
Comment 13 (dormant account) 2007-08-14 11:39:56 PDT
Created attachment 276662 [details] [diff] [review]
Modifications to help outparamdel process mozilla
Comment 14 (dormant account) 2007-08-14 11:42:33 PDT
Created attachment 276663 [details] [diff] [review]
Modifications to help outparamdel process mozilla
Comment 15 (dormant account) 2007-08-14 11:44:20 PDT
Created attachment 276664 [details] [diff] [review]
 Modifications to help outparamdel process mozilla  

I'm having trouble getting this patch to show up as non-empty.
Comment 16 (dormant account) 2007-08-14 14:34:35 PDT
Created attachment 276690 [details] [diff] [review]
Most complete patch yet

(In reply to comment #9)
> Hrm, I don't think there's a good way to put already_AddRefed in the signature,
> because it's untyped... the best you could do is
> already_AddRefed<nsISomeInterface> (nsISomeInterface is a typedef for
> nsISupports that means "any interface"), but that will cause problems with
> nsCOMPtr and friends, I think.
> 
> This code should have been written originally as:
> 
> nsCOMPtr<nsILineIteratorNavigator> it = do_QueryInterface(aBlockFrame);

The patch should have everything except the pattern mentioned above. Let me know if you see any serious issues.
Comment 17 Benjamin Smedberg [:bsmedberg] 2007-08-14 21:13:25 PDT
Taras, the following pattern that used getter_AddRefs need to be changed:

-  QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent));
+  thisContent = (nsIContent*) QueryInterface(NS_GET_IID(nsIContent));

this will cause an extra AddRef. It needs to use something like:

-  QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent));
+  thisContent = QueryInterface(NS_GET_IID(nsIContent));

Which uses the inline templatized version that I wrote which returns an already_AddRefed, or expand it like this (more ugly):

-  QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent));
+  thisContent = getter_AddRefs(static_cast<nsIContent*>(QueryInterface(NS_GET_IID(nsIContent));
Comment 18 Benjamin Smedberg [:bsmedberg] 2007-08-14 21:18:00 PDT
Also, it would be more readable to rewrite statments such as this:

-    return QueryInterface(aIID, aResult);
+    return ((*aResult = (void *) QueryInterface(aIID)) ? NS_OK : NS_ERROR_OUT_OF_MEMORY);

as two statements:

-    return QueryInterface(aIID, aResult);
+    *aResult = QueryInterface(aIID);
+    return *aResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

However, the inline compatibility glue I added to nsISupports really makes this rewriting unnecessary: it's basically un-inlining the function call. Also, avoiding as many casts as pratical (the one above is unnecessary) would be nice for readability.
Comment 19 (dormant account) 2007-08-14 21:22:49 PDT
(In reply to comment #17)
> Taras, the following pattern that used getter_AddRefs need to be changed:
> 
> -  QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent));
> +  thisContent = (nsIContent*) QueryInterface(NS_GET_IID(nsIContent));
> 
> this will cause an extra AddRef. It needs to use something like:
> 
> -  QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent));
> +  thisContent = QueryInterface(NS_GET_IID(nsIContent));
> 
> Which uses the inline templatized version that I wrote which returns an
> already_AddRefed, or expand it like this (more ugly):
> 
> -  QueryInterface(NS_GET_IID(nsIContent), getter_AddRefs(thisContent));
> +  thisContent =
> getter_AddRefs(static_cast<nsIContent*>(QueryInterface(NS_GET_IID(nsIContent));
> 

As I mentioned I haven't gotten around changing this in the current patch. I'll do the pretty version for next patch.
Comment 20 (dormant account) 2007-08-14 21:32:18 PDT
(In reply to comment #18)
> Also, it would be more readable to rewrite statments such as this:
> 
> -    return QueryInterface(aIID, aResult);
> +    return ((*aResult = (void *) QueryInterface(aIID)) ? NS_OK :
> NS_ERROR_OUT_OF_MEMORY);
> 
> as two statements:
> 
> -    return QueryInterface(aIID, aResult);
> +    *aResult = QueryInterface(aIID);
> +    return *aResult ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
> 
> However, the inline compatibility glue I added to nsISupports really makes this
> rewriting unnecessary: it's basically un-inlining the function call. Also,
> avoiding as many casts as pratical (the one above is unnecessary) would be nice
> for readability.
> 

1)I haven't applied your patch as I can't get mozilla-central to build on either of my machines(different, reproducible errors). I'm guessing that there is something wrong with hg mirror as cvs checkouts typically build without trouble here.
Can we agree on a cvs checkout specifying a certain time for doing this? Or perhaps just tarball the current cvs and experiment with that for now?

2)I'm not sure what you mean by unnecessary inlining, could you be more specific? I need to fill in things that expect an nsresults with some valid values for now.

3)I know that the void casts are unnecessary, but at this point I am aiming to have the code compile and run without outparams. Once we are confident that the semantics are being preserved there are a lot of simplifications which can be applied to the code (trimming unused "nsresult rv" variables, simplifying if expressions that check for both return value and outparam..also I rewrite outparam method bodies in quite an ugly way atm)
Comment 21 Benjamin Smedberg [:bsmedberg] 2007-08-15 11:39:28 PDT
Created attachment 276812 [details] [diff] [review]
Full patch against mozilla-central

If you're going to try this against CVS, please use --enable-debug --disable-xmlextras
Comment 22 (dormant account) 2007-08-15 17:14:42 PDT
Benjamin,
I'm trying to apply my patch after yours but I can't easily. Problem is that you rewrote many queryinterface() methods, but not enough to have the code compile(not here). It's not necessary to rewrite those as they should all get covered by my tool, but if I can't compile code I can't use my tool.
Only the code inside macros needs to be rewritten, the rest is ok. Either that or I need to be able to compile code after applying your patch, then the tool can skip calls that have already been rewritten.
For now I'm trying to remove all of the queryinterface() rewrites from your patch, but if you already have a patch without those, that'd be of help.
Comment 23 jag (Peter Annema) 2007-08-16 07:59:20 PDT
I'm working on bug 25886 which will replace some QueryInterface implementations with NS_IMPL_ISUPPORTS, and some direct calls to QueryInterface with {do_,Call}QueryInterface.

bsmedberg, did you run a tool to make your changes? I'm kinda hoping you did so it's easy enough for you to re-run it after I land. Otherwise I'll RPS you for who gets to deal with merging ;-)
Comment 24 (dormant account) 2007-08-22 16:56:48 PDT
Created attachment 277809 [details] [diff] [review]
Modifications to help outparamdel process mozilla

This is a workaround for two bugs in elsa and a switch for two macros to functions so they are easier to deal with.
Comment 25 (dormant account) 2007-08-22 17:01:50 PDT
Created attachment 277811 [details] [diff] [review]
Most complete patch yet

This is a result of running outparamdel on Mozilla. In addition to rewriting QueryInterface, it does AggregatedQueryInterface too. I noticed Benjamin did those manually. 
Benjamin, manual rewriting just makes it harder for me to apply my tools by creating lots of merge conflicts. It's easier if you provide me with a list of non-macro functions that need to be changed. Only stuff within macros should be altered manually.
Comment 26 (dormant account) 2007-08-22 17:14:33 PDT
Created attachment 277816 [details] [diff] [review]
Manual rewriting

This provides the new templated QI call and changes a bunch of macros. There is no backwards-compatible QI call provided.
With this I can compile everything until netwerk/.
There the compiler gets unhappy with the templated QI call with
/home/tglek/work/ff-build/dist/include/xpcom/nsISupportsBase.h: In member function `already_AddRefed<D> nsISupports::QueryInterface() [with D = nsStandardURL]':
/home/tglek/work/mozilla/netwerk/base/src/nsStandardURL.cpp:1586:   instantiated from here
/home/tglek/work/ff-build/dist/include/xpcom/nsISupportsBase.h:98: error: no class template named `COMTypeInfo' in `class nsStandardURL'

I'm not yet sure what's going on. Seems to be caused by the difference between COMPtr pointers and RefPtr ones. I guess we need another templated QI call.
Comment 27 Sergey Yanovich 2007-08-25 05:04:59 PDT
(In reply to comment #26)
> I'm not yet sure what's going on. Seems to be caused by the difference between
> COMPtr pointers and RefPtr ones. I guess we need another templated QI call.

AFAICR RefPtr is product of deCOM campaign, and is a COMPtr version with QI stuff stripped of. I this may the reason, why 'QueryInterface' is called here directly, not via 'do_QueryInteface'. And this a part of troubles (or challenges) bsmedberg predicted in comment #9. In addition, I would expect issues in spidermonkey, since it is in c, not c++.

I might have a plan how to avoid this kind of issues all together. In short, QI should not AddReff.

QI is a fundamental part of XPCOM, and after this bug is landed, I would like to be able to have code like that:

virtual bar
Class::GetBar() { return mFoo.QueryInterface(NS_GET_IID(BarFetcher)).GetBar();}

There are obviously two global reference counting usage patterns: callee AddRefs and caller AddRefs. Mozilla is currently using the formar. Among other things the current model guaranties a leak in the above example.

I infer that outparam was introduced in the early days of mozilla to fight this exact type of leak. 'AlreadyAddrefed' was invented later to allow pointer in return value, but at the same time it attempts to ensure that the return value is stored to a smart pointer. Since there are no smart pointers in c, I mentioned spidermonkey above.

Of course, the proposed model is no silver bullet. It will also cause a leak, if 'mFoo' is replaced with 'CreateFoo()' call in the example. In addition, if a pointer with a zero refcount is ever assigned to a smart pointer, 'delete' will be called on it when the smart pointer goes out of scope.

There might be answers to both issues:
1. All classes that implement QI have a single place in which they are created atm. It is normally a factory. We can leverage mozilla threading framework, and add a 'autoclean' or something parameter to factory calls. If the parameter is true, the factory will AddRef and post a message to the current thread to release the created object. If the object is stored somewhere, its refcount will be greater than 1, and the object won't be deleted.

2. Dealing with smart pointers will require a new ISupports method, it can be called 'UnRef()'. This method will decrease refcount, but will NOT cause automatic self-destruction.

Usage of 'UnRef' would not be very safe, and to achieve its goal 'UnRef' must be called AFTER the smart pointer goes out of scope, which is tricky. This can be dealt with 'ResultPtr<T>' - a special type of smart pointer, which is similar to 'nsCOMPtr<T>' in all senses, except that it calls 'UnRef()' instead of 'Release()' in its destructor. Since there is no more than one return value per functions, automatic code analysis tools will easily detect and fix 'ResultPtr' misuses.
Comment 28 Sergey Yanovich 2007-08-25 05:25:07 PDT
A few words on refactoring strategy.

If I were you, I would add new method like 'AskInterface' and would call it inside  'QueryInterface'. Then, I would be gradually replacing 'QueryInterface' calls with 'AskInterface' calls. That way I would be certain to compile-test after every step. After I had a 95% coverage, I would land the patch and forbid QI usage in new cvs commits. Finally, I would merge a patch renaming 'AI' to 'QI' (or even leave the name for its cool abbreviation and drop 'QI' :).
Comment 29 (dormant account) 2007-08-30 14:33:07 PDT
Created attachment 279007 [details] [diff] [review]
patch #2: Autogenerated qi rewrite

This is for yesterday's trunk. Fairly straight-forward to keep updated.
I didn't want to fight C++ method overloads so I changed the templated QI to have a different name.
-       xulWindow->QueryInterface(NS_GET_IID(nsXULWindow), getter_AddRefs(win));
+       xulWindow->QueryInterfaceAAR<nsXULWindow>(NS_GET_IID(nsXULWindow));

Is what it looks like. When someone figures out a better way, it's pretty simple to change outparamdel accordingly. I think AAR is supposed to mean AlreadyAddRefed.
Comment 30 (dormant account) 2007-08-30 14:34:40 PDT
Created attachment 279009 [details] [diff] [review]
patch #3: Template fixup

Fixes the autogenerated patch to not resolve templated-types.
Comment 31 (dormant account) 2007-08-30 14:37:42 PDT
Created attachment 279010 [details] [diff] [review]
patch #4: Workaround for mcpp macro-annotation bug. This will be fixed soon.
Comment 32 (dormant account) 2007-08-30 14:40:57 PDT
Created attachment 279011 [details] [diff] [review]
patch #5: Manual macro-rewriting

There are issues called by multiple inheritance with compiler getting confused as to which QI method to call. This tried to manually correct some of those. I probably got at least one of those wrong. I disabled accessibility in my build because that code has a lot of those issues that I didn't want to deal with.

With this, the trunk compiles.
Comment 33 (dormant account) 2007-08-30 15:05:21 PDT
Created attachment 279015 [details] [diff] [review]
build config. had to disable a couple of things since they give mcpp trouble.
Comment 34 neil@parkwaycc.co.uk 2007-08-31 02:59:28 PDT
template<class D> NS_HIDDEN_(already_AddRefed<D>)
  QueryInterfaceAAR(REFNSIID aIID) {
  return static_cast<D*>(QueryInterface(aIID));
}

How do you get this to compile? Surely you'll always get error C2783: 'struct already_AddRefed<D> nsISupports::QueryInterfaceD(nsID &)' : could not deduce template argument for 'D' or some such?
Comment 35 Sergey Yanovich 2008-05-22 01:42:05 PDT
What's the current state of affairs here?

Mozilla 2 meeting on 2008-04-16 [1] states that this QI rewrite introduces 5% performance gain. Is there more info on this topic?

1. http://wiki.mozilla.org/Mozilla_2/StatusMeetings/2008-04-16
Comment 36 Benjamin Smedberg [:bsmedberg] 2008-05-22 06:58:21 PDT
I'm not going to resurrect this project until we can break XPCOM compatibility, which is after 1.9.1 branches...
Comment 37 Justin Lebar (not reading bugmail) 2011-05-18 12:09:57 PDT
Taras / bsmedberg, is there any plan to move forward with this, now that we can break xpcom compat?
Comment 38 Benjamin Smedberg [:bsmedberg] 2011-05-18 12:11:31 PDT
This sounds like a *great* summer intern project. I personally am not sure I'll have the time to take this on soon.
Comment 39 (dormant account) 2011-05-18 14:46:57 PDT
(In reply to comment #37)
> Taras / bsmedberg, is there any plan to move forward with this, now that we
> can break xpcom compat?

No plans. The rewriting toolchain bitrotted a fair bit, but this should be doable via clang relatively easily.

(In reply to comment #38)
> This sounds like a *great* summer intern project. I personally am not sure
> I'll have the time to take this on soon.

I agree, this would be a very fun intern project.
Comment 40 Dirkjan Ochtman (:djc) 2011-05-19 00:57:25 PDT
(In reply to comment #39)
> No plans. The rewriting toolchain bitrotted a fair bit, but this should be
> doable via clang relatively easily.

Can you elaborate on this? Does clang no come with rewriting facilities?
Comment 41 Benjamin Smedberg [:bsmedberg] 2011-05-19 06:23:18 PDT
I think we could probably get the old oink stack to work for this task without too much effort: it worked once, so we'd mainly be dealing with new code patterns that oink doesn't recognize, which shouldn't be too bad.

clang IR comes with very precise location information, but we'd have to develop our own rewriting tools on top of it.
Comment 42 Justin Lebar (not reading bugmail) 2011-05-19 09:59:50 PDT
As a 90% solution, why not:

 * Add QueryInterfaceFast which returns a void*.
 * Make QueryInterface a wrapper around QueryInterfaceFast.  (I'm not totally familiar with the class structure, but we might even be able to get away with making it non-virtual on nsISupports.)
 * Modify all the QI wrappers (do_QueryInterface, etc.) to use QueryInterfaceFast.
 * Modify by hand the few places which use QueryInterface directly and which are performance-sensitive to use QueryInterfaceFast or a wrapper.

We could also rename QueryInterface to QueryInterfaceSlow and make the new function QueryInterface.

If all the automatic rewriting buys us is the cleanness of not having two functions, I'm not convinced it's worth the effort.  Especially since it looks like the resulting code may be at least a little ugly.
Comment 43 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-19 20:50:34 PDT
Created attachment 561115 [details] [diff] [review]
New approach

This patch implements Justin's idea in comment 42.  Here's a short description of what the patch does:

1. It replaces nsISupports::QueryInterface with |void* nsISupports::QueryInterfaceFast(const nsID&)|.  It rewrites all of the implementations of this function in the code base with the new signatures with this change in the semantics: if the requested interface cannot be obtained for whatever reason, null is returned.  Otherwise, a pointer to the requested interface will be returned.

2. It implements |nsresult nsISupports::QueryInterface(const nsID&, void**)| as a backwards compatibility shim as an inline non-virtual method on nsISupports, which wraps QueryInterfaceFast.  This enables almost all legacy code calling nsISupports::QueryInterface directly to compile just fine (see item 4 below for the only exception here).

3. It implements the do_QueryInterface and CallQueryInterface APIs to use nsISupports::QueryInterfaceFast.

4. The only case where the new nsISupports::QueryInterface shim fails is when QueryInterface is called on a concrete type inheriting from nsISupports more than once.  There were multiple approaches to addressing the problem, but I decided that the cleanest (and hardest for me!) would be to just fix those callsites to use QueryInterfaceFast manually, so I did that.  This is a large portion of the patch.

5. During step 4, I came across a bunch of things which were easy and made sense to do, and I did them.  (Most notable examples: making the interfaces to the do_XXX APIs uniform.)  None of this stuff should have any backwards compatibility issues (well, for sane code anyways).


In case this is not obvious, THIS PATCH WILL BREAK SOURCE _AND_ BINARY XPCOM COMPATIBILITY!  Which means that every binary extension which uses XPCOM in anyway needs to be recompiled at least.  Item 4 above makes this patch to not preserve source compatibility, which means that the client code may need to be rewritten (I can write up some docs on that if needed, as the patterns are pretty straightforward.)  FWIW, it took me about two days to fix all problematic cases, using grep and the try server for all non-mac platforms that we support (Linux, Linux64, LinuxQt, Win32, Win64, Android-r7, Maemo, MeamoQt.)

This means that this patch will break almost all binary add-ons.  If plugins are capable of accessing XPCOM stuff (I don't know if that's the case or not), then they will be affected too.

Another change here is that nsISupports will not be binary compatible with IUnknown any more.  I think the only place in our codebase where we rely on this is in our a11y code on Windows, but that's something that we need to fix IMHO.  I've made a bunch of changes there to get the patch to compile, but I'm pretty sure that some of that stuff needs to be rewritten.

Benjamin, I'm really eager to see what you think about this.  :-)
Comment 44 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-19 20:51:31 PDT
David, Alex, please see comment 43 (especially its last paragraph).
Comment 45 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-19 20:53:11 PDT
Oh, and I failed to mention another obvious thing: this patch changes the IID of nsISupports!
Comment 46 alexander :surkov 2011-09-19 22:23:10 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #44)
> David, Alex, please see comment 43 (especially its last paragraph).

We shouldn't have any problems here because we never query nsISupports to IUnknown and vice versa. We query nsISupports and IUnknown form objects that implement these interfaces. Btw, your patch seems to do wrong things (like in ia2AccessibleRelation) assuming that. And btw, for atk part you should use do_QueryInterface instead.
Comment 47 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 07:06:45 PDT
(In reply to alexander surkov from comment #46)
> (In reply to Ehsan Akhgari [:ehsan] from comment #44)
> > David, Alex, please see comment 43 (especially its last paragraph).
> 
> We shouldn't have any problems here because we never query nsISupports to
> IUnknown and vice versa. We query nsISupports and IUnknown form objects that
> implement these interfaces.

That doesn't quite seem to be the case!  Specifically, see <https://bugzilla.mozilla.org/attachment.cgi?id=561115&action=diff#a/accessible/src/msaa/ia2AccessibleRelation.cpp_sec1>, where we QI for IUnknown using nsISupports::QueryInterface (note that before this patch, that would succeed since the IID of IUnknown and nsISupports are the same, and the vtable entires are the same and have the same signature.)

There might also be other cases like this that didn't fail to compile, but will fail at runtime because of the IID change in nsISupports.

> Btw, your patch seems to do wrong things (like in ia2AccessibleRelation) assuming that.

Yes, that was what I was talking about.  The correct fix there would be to call the MSCOM QI if we're querying for MSCOM interfaces.  But I don't know enough about the accessibility code to fix that myself.

> And btw, for atk part you should use do_QueryInterface instead.

Probably, yes.  But I'll wait for Benjamin's feedback before spending more time on this.
Comment 48 Joshua Cranmer [:jcranmer] 2011-09-20 09:55:11 PDT
Another thing to watch out for is if we use any of our nsISupports helpers to work with MSCOM interfaces.

As another addendum, if you're changing the vtable of the nsISupports, please also change the code in nsISupports.idl to reflect the proper IDL version of it.
Comment 49 Mozilla RelEng Bot 2011-09-20 10:10:56 PDT
Try run for 22a581406f7c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=22a581406f7c
Results (out of 539 total builds):
    exception: 7
    success: 503
    warnings: 27
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-22a581406f7c
Comment 50 (dormant account) 2011-09-20 10:15:54 PDT
Ehsan, were you able to measure any performance/footprint improvements?
Comment 51 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 12:43:18 PDT
(In reply to Joshua Cranmer [:jcranmer] from comment #48)
> Another thing to watch out for is if we use any of our nsISupports helpers
> to work with MSCOM interfaces.

That should fail to compile now, except for the helpers that use the old-style QI, which we don't need to worry about.

> As another addendum, if you're changing the vtable of the nsISupports,
> please also change the code in nsISupports.idl to reflect the proper IDL
> version of it.

Well, things have not changed as far as js consumers are concerned.  I'm not sure how I should reflect that change in the IDL file.
Comment 52 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 13:08:14 PDT
(In reply to Taras Glek (:taras) from comment #50)
> Ehsan, were you able to measure any performance/footprint improvements?

Yes, see http://mzl.la/onXyUo.  We're winning about 3-5% on the Dromaeo DOM stuff, which is what I was hoping for.
Comment 53 Justin Lebar (not reading bugmail) 2011-09-20 13:17:47 PDT
Dromaeo is a bigger-is-better test.  The only dromaeo test which shows an improvement is Dromaeo V8, and it's likely noise...
Comment 54 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 13:25:35 PDT
(In reply to Justin Lebar [:jlebar] from comment #53)
> Dromaeo is a bigger-is-better test.  The only dromaeo test which shows an
> improvement is Dromaeo V8, and it's likely noise...

Ah, I thought that compare-talos handles that correctly, so I mainly looked at the percentage changes...

So given that, this is WONTFIX, right?
Comment 55 Joshua Cranmer [:jcranmer] 2011-09-20 13:33:08 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #51)
> > As another addendum, if you're changing the vtable of the nsISupports,
> > please also change the code in nsISupports.idl to reflect the proper IDL
> > version of it.
> 
> Well, things have not changed as far as js consumers are concerned.  I'm not
> sure how I should reflect that change in the IDL file.

[noscript, notxpcom] nsQIResult QueryInterface(in nsIIDRef uuid);

That is probably the way to go. It shouldn't be a problem for JS, since I think xpconnect already special-cases QueryInterface, but having the proper ABI reflected in IDL is nice.
Comment 56 (dormant account) 2011-09-20 13:39:31 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #54)
> (In reply to Justin Lebar [:jlebar] from comment #53)
> > Dromaeo is a bigger-is-better test.  The only dromaeo test which shows an
> > improvement is Dromaeo V8, and it's likely noise...
> 
> Ah, I thought that compare-talos handles that correctly, so I mainly looked
> at the percentage changes...
> 
> So given that, this is WONTFIX, right?

Given the huge perf differences, i'd check the performance by hand. I don't see why dom/css would be slower when you have less code running.
Comment 57 Benjamin Smedberg [:bsmedberg] 2011-09-20 13:46:47 PDT
What does "proper ABI reflected in the IDL" mean? Isn't that an accurate description of the ABI?

I'm pretty surprised that this shows *no* wins, especially in startup time where it showed a consistent 2-2.5% win in the prior iteration which should be mostly the same code.
Comment 58 Justin Lebar (not reading bugmail) 2011-09-20 13:50:06 PDT
The problem is that the variance you're seeing on m-c (left columns) is *huge*.  Have a look at Dromaeo CSS:

  WinXP, min 2656, max 3130.

No way you're going to be able to tell how you're improving perf with a range like that.
Comment 59 Justin Lebar (not reading bugmail) 2011-09-20 13:54:22 PDT
Ah, so it looks like something landed recently which changed the Dromaeo results a lot, and the changesets you were testing against span that change.

See http://people.mozilla.org/~jlebar/graphs/graph.html#tests=[[73,1,14],[73,1,1]]&sel=none&displayrange=90&datatype=running
Comment 60 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 14:11:08 PDT
(In reply to Justin Lebar [:jlebar] from comment #59)
> Ah, so it looks like something landed recently which changed the Dromaeo
> results a lot, and the changesets you were testing against span that change.
> 
> See
> http://people.mozilla.org/~jlebar/graphs/graph.html#tests=[[73,1,14],[73,1,
> 1]]&sel=none&displayrange=90&datatype=running

Bah, you're right!  Would it make sense for me to rebase my patch on top of a revision before that?

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #57)
> What does "proper ABI reflected in the IDL" mean? Isn't that an accurate
> description of the ABI?
> 
> I'm pretty surprised that this shows *no* wins, especially in startup time
> where it showed a consistent 2-2.5% win in the prior iteration which should
> be mostly the same code.

I'm going to test pushing a dummy thing on the same base revision to try and see how much noise we're really dealing with.
Comment 61 Justin Lebar (not reading bugmail) 2011-09-20 14:46:26 PDT
> Bah, you're right!  Would it make sense for me to rebase my patch on top of a revision before that?

Here are the compare-talos results excluding the new m-c changesets: http://goo.gl/itQaN

There are some moderate Dromaeo gains.  ts appears pretty flat.
Comment 62 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-20 15:08:31 PDT
The try change I talked about in comment 60 is https://tbpl.mozilla.org/?tree=Try&rev=7e4ac078c04b.  We'll have more results on how bogus our Talos infrastructure is soon.  :-)
Comment 63 alexander :surkov 2011-09-20 19:34:41 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #47)
> > Btw, your patch seems to do wrong things (like in ia2AccessibleRelation) assuming that.
> 
> Yes, that was what I was talking about.  The correct fix there would be to
> call the MSCOM QI if we're querying for MSCOM interfaces.  But I don't know
> enough about the accessibility code to fix that myself.

Just call IUnknown::QueryInterface there, after your patch it gives correct results. This works because it's called on nsAccessible which is inherited from nsAccessNodeWrap which implements IUnknown.
Comment 64 Mozilla RelEng Bot 2011-09-21 04:02:25 PDT
Try run for 7e4ac078c04b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7e4ac078c04b
Results (out of 70 total builds):
    exception: 2
    success: 63
    warnings: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-7e4ac078c04b
Comment 65 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-21 10:21:01 PDT
So, the try server changes today will ruin all of my perf number hunting jobs on try.  I need to post new try server jobs today.  But until then, here's some information for you to see:

ehsanakhgari:~/moz/tmp [01:14:16]$ size obj-ff-opt{,-qi}/dist/lib/XUL 
__TEXT	__DATA	__OBJC	others	dec	hex
25616384	3108864	0	24420352	53145600	32af000	obj-ff-opt/dist/lib/XUL
25587712	3108864	0	24436736	53133312	32ac000	obj-ff-opt-qi/dist/lib/XUL

This patch saves 28KB of code in libxul.  (Not sure if I should be disappointed by that or not!)
Comment 66 Mozilla RelEng Bot 2011-09-21 15:41:03 PDT
Try run for 0c205be7c1b9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0c205be7c1b9
Results (out of 61 total builds):
    exception: 1
    success: 57
    warnings: 2
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-0c205be7c1b9
Comment 67 Mozilla RelEng Bot 2011-09-22 00:12:02 PDT
Try run for 8d710229e5e0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8d710229e5e0
Results (out of 70 total builds):
    success: 68
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-8d710229e5e0
Comment 68 Mozilla RelEng Bot 2011-09-22 21:51:09 PDT
Try run for d78fa556fcd9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d78fa556fcd9
Results (out of 310 total builds):
    success: 308
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-d78fa556fcd9
Comment 69 Mozilla RelEng Bot 2011-09-23 01:51:11 PDT
Try run for e6c6e8fe5ad7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e6c6e8fe5ad7
Results (out of 275 total builds):
    exception: 4
    success: 267
    warnings: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-e6c6e8fe5ad7
Comment 70 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-23 07:04:07 PDT
Here is another perf measurement that I did, this time entirely on try server based on the same revision: http://mzl.la/pMViIp

It seems like there is no meaningful performance gains with this patch.  :(
Comment 71 Benjamin Smedberg [:bsmedberg] 2011-10-10 10:27:48 PDT
Comment on attachment 561115 [details] [diff] [review]
New approach

If there is no meaningful change :-((( then we shouldn't do this.

Note You need to log in before you can comment on or make changes to this bug.