Closed Bug 391275 Opened 17 years ago Closed 13 years ago

Remove the QueryInterface outparam semantics (change its signature to void * QueryInterface(REFNSIID iid))

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: taras.mozilla, Assigned: ehsan.akhgari)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file, 17 obsolete files)

292.14 KB, patch
benjamin
: review-
Details | Diff | Splinter Review
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
Attached patch first attempt (obsolete) — Splinter Review
This the first attempt, some things may not work right. I have to fix some dereferencing of void* casts.
Attached file List of macros to rewrite (obsolete) —
This is a list of macros. I can't rewrite these automatically. These will likely have to be done by hand.
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))))
(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. 
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.
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.
Status: NEW → ASSIGNED
Attached patch Cleaner attempt (obsolete) — Splinter Review
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.
Attachment #275694 - Attachment is obsolete: true
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).
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.
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.
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.
Attached patch Manual rewriting, rev. 1 (obsolete) — Splinter Review
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).
Attachment #276662 - Attachment is obsolete: true
I'm having trouble getting this patch to show up as non-empty.
Attachment #276663 - Attachment is obsolete: true
Attached patch Most complete patch yet (obsolete) — Splinter Review
(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.
Attachment #276056 - Attachment is obsolete: true
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));
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.
(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.
(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)
If you're going to try this against CVS, please use --enable-debug --disable-xmlextras
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.
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 ;-)
This is a workaround for two bugs in elsa and a switch for two macros to functions so they are easier to deal with.
Attachment #276664 - Attachment is obsolete: true
Attached patch Most complete patch yet (obsolete) — Splinter Review
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.
Attachment #276690 - Attachment is obsolete: true
Attached patch Manual rewriting (obsolete) — Splinter Review
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.
Attachment #276520 - Attachment is obsolete: true
(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.
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' :).
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.
Attachment #277811 - Attachment is obsolete: true
Attached patch patch #3: Template fixup (obsolete) — Splinter Review
Fixes the autogenerated patch to not resolve templated-types.
Attachment #279009 - Attachment description: patch #3 → patch #3: Template fixup
Attached patch patch #5: Manual macro-rewriting (obsolete) — Splinter Review
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.
Attachment #275696 - Attachment is obsolete: true
Attachment #276812 - Attachment is obsolete: true
Attachment #277816 - Attachment is obsolete: true
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?
Depends on: 395444
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
I'm not going to resurrect this project until we can break XPCOM compatibility, which is after 1.9.1 branches...
Depends on: 435431
Taras / bsmedberg, is there any plan to move forward with this, now that we can break xpcom compat?
This sounds like a *great* summer intern project. I personally am not sure I'll have the time to take this on soon.
(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.
(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?
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.
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.
Attached patch New approachSplinter Review
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.  :-)
Assignee: tglek → ehsan
Attachment #277809 - Attachment is obsolete: true
Attachment #279007 - Attachment is obsolete: true
Attachment #279009 - Attachment is obsolete: true
Attachment #279010 - Attachment is obsolete: true
Attachment #279011 - Attachment is obsolete: true
Attachment #279015 - Attachment is obsolete: true
Attachment #561115 - Flags: review?(benjamin)
David, Alex, please see comment 43 (especially its last paragraph).
Summary: Use oink to change QueryInterface to void * QueryInterface(REFNSIID iid); → Remove the QueryInterface outparam semantics (change its signature to void * QueryInterface(REFNSIID iid))
Oh, and I failed to mention another obvious thing: this patch changes the IID of nsISupports!
(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.
(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.
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.
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
Ehsan, were you able to measure any performance/footprint improvements?
(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.
(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.
Dromaeo is a bigger-is-better test.  The only dromaeo test which shows an improvement is Dromaeo V8, and it's likely noise...
(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?
(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.
(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.
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.
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.
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
(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.
> 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.
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.  :-)
Depends on: 662693
(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.
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
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!)
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
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
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
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
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 on attachment 561115 [details] [diff] [review]
New approach

If there is no meaningful change :-((( then we shouldn't do this.
Attachment #561115 - Flags: review?(benjamin) → review-
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: