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)
Core
XPCOM
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
Reporter | ||
Comment 1•17 years ago
|
||
This the first attempt, some things may not work right. I have to fix some dereferencing of void* casts.
Reporter | ||
Comment 2•17 years ago
|
||
This is a list of macros. I can't rewrite these automatically. These will likely have to be done by hand.
Comment 3•17 years ago
|
||
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))))
Reporter | ||
Comment 4•17 years ago
|
||
(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•17 years ago
|
||
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.
Reporter | ||
Comment 6•17 years ago
|
||
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
Reporter | ||
Comment 7•17 years ago
|
||
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
Reporter | ||
Comment 8•17 years ago
|
||
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•17 years ago
|
||
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.
Comment 11•17 years ago
|
||
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•17 years ago
|
||
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).
Reporter | ||
Comment 13•17 years ago
|
||
Reporter | ||
Comment 14•17 years ago
|
||
Attachment #276662 -
Attachment is obsolete: true
Reporter | ||
Comment 15•17 years ago
|
||
I'm having trouble getting this patch to show up as non-empty.
Attachment #276663 -
Attachment is obsolete: true
Reporter | ||
Comment 16•17 years ago
|
||
(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
Comment 17•17 years ago
|
||
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•17 years ago
|
||
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.
Reporter | ||
Comment 19•17 years ago
|
||
(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.
Reporter | ||
Comment 20•17 years ago
|
||
(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•17 years ago
|
||
If you're going to try this against CVS, please use --enable-debug --disable-xmlextras
Reporter | ||
Comment 22•17 years ago
|
||
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•17 years ago
|
||
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 ;-)
Reporter | ||
Comment 24•17 years ago
|
||
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
Reporter | ||
Comment 25•17 years ago
|
||
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
Reporter | ||
Comment 26•17 years ago
|
||
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
Comment 27•17 years ago
|
||
(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•17 years ago
|
||
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' :).
Reporter | ||
Comment 29•17 years ago
|
||
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
Reporter | ||
Comment 30•17 years ago
|
||
Fixes the autogenerated patch to not resolve templated-types.
Reporter | ||
Updated•17 years ago
|
Attachment #279009 -
Attachment description: patch #3 → patch #3: Template fixup
Reporter | ||
Comment 31•17 years ago
|
||
Reporter | ||
Comment 32•17 years ago
|
||
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
Reporter | ||
Comment 33•17 years ago
|
||
Comment 34•17 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
I'm not going to resurrect this project until we can break XPCOM compatibility, which is after 1.9.1 branches...
Comment 37•13 years ago
|
||
Taras / bsmedberg, is there any plan to move forward with this, now that we can break xpcom compat?
Comment 38•13 years ago
|
||
This sounds like a *great* summer intern project. I personally am not sure I'll have the time to take this on soon.
Reporter | ||
Comment 39•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 43•13 years ago
|
||
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)
Assignee | ||
Comment 44•13 years ago
|
||
David, Alex, please see comment 43 (especially its last paragraph).
Assignee | ||
Updated•13 years ago
|
Summary: Use oink to change QueryInterface to void * QueryInterface(REFNSIID iid); → Remove the QueryInterface outparam semantics (change its signature to void * QueryInterface(REFNSIID iid))
Assignee | ||
Comment 45•13 years ago
|
||
Oh, and I failed to mention another obvious thing: this patch changes the IID of nsISupports!
Comment 46•13 years ago
|
||
(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.
Assignee | ||
Comment 47•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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
Reporter | ||
Comment 50•13 years ago
|
||
Ehsan, were you able to measure any performance/footprint improvements?
Assignee | ||
Comment 51•13 years ago
|
||
(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.
Assignee | ||
Comment 52•13 years ago
|
||
(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•13 years ago
|
||
Dromaeo is a bigger-is-better test. The only dromaeo test which shows an improvement is Dromaeo V8, and it's likely noise...
Assignee | ||
Comment 54•13 years ago
|
||
(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•13 years ago
|
||
(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.
Reporter | ||
Comment 56•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 60•13 years ago
|
||
(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•13 years ago
|
||
> 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.
Assignee | ||
Comment 62•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
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
Assignee | ||
Comment 65•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 70•13 years ago
|
||
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•13 years ago
|
||
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-
Assignee | ||
Updated•13 years ago
|
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.
Description
•