Closed
Bug 849567
Opened 11 years ago
Closed 10 years ago
Automatically handle raw and already_AddRefed return values in Paris bindings and remove resultNotAddRefed
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: smaug, Assigned: peterv)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files, 13 obsolete files)
8.20 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.28 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
13.95 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
34.07 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Would be easier to add webidl bindings if one could add the perhaps most commonly used annotations to .webidl. It would make also reading the code easier. No need to check .conf to find which C++ class implements the interface.
Reporter | ||
Comment 1•11 years ago
|
||
Especially resultNotAddRefed part would be really useful with events, since I won't be adding all the (generated) events to .conf and would like to avoid generating .conf based on the list of generated events. ...and still would like to use faster return values when possible. For now I've on purpose just not used resultNotAddRefed with events.
Reporter | ||
Comment 2•11 years ago
|
||
Needs some macro magic to make it work, but hopefully patch coming.
Assignee: nobody → bugs
Summary: Move nativeType and resultNotAddRefed from .conf to .webidl → Automatically handle raw and already_AddRefed return values in Paris bindings and remove resultNotAddRefed
Reporter | ||
Comment 3•11 years ago
|
||
Reporter | ||
Comment 4•11 years ago
|
||
I guess my SFINAE is still wrong.
Comment 5•11 years ago
|
||
Why do we need SFINAE here at all?
Reporter | ||
Comment 6•11 years ago
|
||
Because we get for example StrongOrRawPtr<JSObject> and JSObject certainly doesn't have AddRef/Release methods.
Reporter | ||
Comment 7•11 years ago
|
||
Hmm, but now that you ask... why do we generate such code.
Reporter | ||
Comment 8•11 years ago
|
||
It is because of workers.
Reporter | ||
Comment 9•11 years ago
|
||
This might work https://tbpl.mozilla.org/?tree=Try&rev=c35c6d5ea771
Attachment #723775 -
Attachment is obsolete: true
Reporter | ||
Comment 10•11 years ago
|
||
Tryserver totally does not like the patch
Reporter | ||
Comment 11•11 years ago
|
||
because it is missing a null check. ugh.
Reporter | ||
Comment 12•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4f40648c9071
Attachment #723896 -
Attachment is obsolete: true
Comment 13•11 years ago
|
||
Instead of using .workers checks, can we use the .nativeOwnership of the relevant descriptor? That's the right thing to do, it seems to me. And yes, I know that's not what isResultAlreadyAddRefed does... but I'm looking forward to when we start fixing the workers ownership model.
Reporter | ||
Comment 14•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=86bba39f175c The previous patch worked, so perhaps this one passes tests too.
Attachment #723904 -
Attachment is obsolete: true
Attachment #723981 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 15•11 years ago
|
||
So, we could just split the methods implementing getters to smaller pieces so that there is always one internal method which takes Foo* and one which takes already_AddRefed<Foo> as param. I'll try to figure out how to change codegen to do that.
Comment 16•11 years ago
|
||
I'm not sure I follow comment 15. What exactly is the proposal?
Reporter | ||
Updated•11 years ago
|
Attachment #723981 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 17•11 years ago
|
||
Currently the generated code is something like (Type is either Foo* or nsRefPtr<Foo>) bool get_foo(JSContext* cx, JSHandleObject obj, SomeType* self, JS::Value* vp) { Type result; result = self->GetFoo(); ...do something with result; } We could have bool get_foo(JSContext* cx, JSHandleObject obj, SomeType* self, JS::Value* vp) { return get_foo_internal(cx, obj, self->GetFoo(), vp) } MOZ_ALWAYS_INLINE bool get_foo_internal(JSContext* cx, JSHandleObject obj, alreadyAddRefed<Type>& r, JS::Value* vp) { nsRefPtr<Type> result = r; return get_foo_internal(cx, obj, r, vp); } MOZ_ALWAYS_INLINE bool get_foo_internal(JSContext* cx, JSHandleObject obj, Type* r, JS::Value* vp) { ...do something with result; }
Comment 18•11 years ago
|
||
Ah, I see. Hmm. That might work pretty well for getters; doing it for methods might be a bit more of a pain. Maybe.
Comment 19•11 years ago
|
||
I think we should just do the branch and pay whatever the tiny perf hit is. The usability benefits are worth it.
Reporter | ||
Comment 21•11 years ago
|
||
Feel free to take this, but I don't think we should take the branch approach.
Flags: needinfo?(bugs)
Comment 22•11 years ago
|
||
Why not? The measured hit was pretty small, and the maintainability benefits are pretty nice...
Comment 23•11 years ago
|
||
Attachment #723981 -
Attachment is obsolete: true
Attachment #744385 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 24•11 years ago
|
||
Because the performance hit happens in so many cases. It shouldn't be too hard to bend the code generator to do the right thing.
Comment 25•11 years ago
|
||
So I tried re-measuring a microbenchmark. In particular, I tried commenting out the [Pure] on Node.firstChild and then measuring like so: <div id="x"></div> <pre><script> var x = document.getElementById("x"); var y = document.documentElement; var count = 100000000; var start = new Date; for (var i = 0; i < count; ++i) { x.firstChild; } var stop = new Date; document.writeln((stop - start) / count * 1e6); start = new Date; for (var i = 0; i < count; ++i) { y.firstChild; } stop = new Date; document.writeln((stop - start) / count * 1e6); </script> The time for "x" does not seem to change with this patch. The time for "y" goes from about 11.5ns to about 14ns. This confuses me, since the cost of the dtor should be the same both ways...
Comment 26•11 years ago
|
||
pass and already_AddRefed to one of the wrap methods and end up just calling get() on it and leaking.
Attachment #751984 -
Flags: review?(bugs)
Updated•11 years ago
|
Assignee: bugs → bzbarsky
Comment 27•11 years ago
|
||
Comment on attachment 744385 [details] [diff] [review] Updated patch Let's see if we can avoid the perf hit.
Attachment #744385 -
Flags: review?(bzbarsky) → review-
Comment 28•11 years ago
|
||
function so we can create separate overloads for pointers and already_AddRefed. Then we just need lots of overloads to cover all the annoying cases.
Attachment #751987 -
Flags: review?(bugs)
Comment 29•11 years ago
|
||
I tried to figure out a cleaner way to do part 2, and failed. :( I could _maybe_ get rid of a tiny bit of code if I merge CGCallGenerator and CGPerSignatureCall. Maybe.
Comment 30•11 years ago
|
||
Attachment #751988 -
Flags: review?(bugs)
Updated•11 years ago
|
Whiteboard: [need review]
Comment 31•11 years ago
|
||
Comment on attachment 751987 [details] [diff] [review] part 2. Stop using alreadyAddReffed information in codegen. just pass the return value of our C++ call directly to a This is wrong; it fails to do the right thing for nodelist getters for example.
Attachment #751987 -
Flags: review?(bugs) → review-
Reporter | ||
Updated•11 years ago
|
Attachment #751988 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 32•11 years ago
|
||
Comment on attachment 751984 [details] [diff] [review] part 1. Make sure to not treat already_AddRefed as a smartptr by accident. is very hacky, but I want to be extra-sure we don't accidentally Not too pretty.
Attachment #751984 -
Flags: review?(bugs) → review+
Updated•11 years ago
|
Assignee: bzbarsky → nobody
Whiteboard: [need review]
Assignee | ||
Comment 33•10 years ago
|
||
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #744385 -
Attachment is obsolete: true
Attachment #751984 -
Attachment is obsolete: true
Attachment #751987 -
Attachment is obsolete: true
Attachment #751988 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Assignee | ||
Comment 36•10 years ago
|
||
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8484435 -
Attachment is obsolete: true
Comment 39•10 years ago
|
||
StrongOrRawPtr should probably have an overload taking nsRefPtr too, right? Otherwise functions that currently return one will get the raw-pointer thing...
Assignee | ||
Comment 40•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #39) > StrongOrRawPtr should probably have an overload taking nsRefPtr too, right? > Otherwise functions that currently return one will get the raw-pointer > thing... Hmm, good point. But I wonder if we shouldn't just disallow that? Unless I'm missing something, if you're planning on returning nsRefPtr then it's fairly simple to return already_AddRefed instead. How about this: template<class T, class S> inline nsRefPtr<T> StrongOrRawPtr(already_AddRefed<S>&& aPtr) { return nsRefPtr<T>(Move(aPtr)); } template<class T> inline T* StrongOrRawPtr(T* aPtr) { return aPtr; } template<class T, template<typename> class SmartPtr, class S> inline void StrongOrRawPtr(SmartPtr<S>&& aPtr) MOZ_DELETE; I verified that it blocks returning nsRefPtr<...>, and luckily it seems nothing in the tree currently does that.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 41•10 years ago
|
||
BTW, the codegen explicitly specifies the value of T, so that we can avoid ending up with totally unrelated types of objects.
Comment 42•10 years ago
|
||
> But I wonder if we shouldn't just disallow that? Sounds great to me! > BTW, the codegen explicitly specifies the value of T Can we still have a method declared in IDL as returning a Node actually return an Element or already_AddRefed<Element>? As long as we can, great.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 43•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #42) > Can we still have a method declared in IDL as returning a Node actually > return an Element or already_AddRefed<Element>? As long as we can, great. Yeah, that's why I had to add the |class S| template argument.
Assignee | ||
Comment 44•10 years ago
|
||
Attachment #8484438 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8484433 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8484434 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 45•10 years ago
|
||
Attachment #8485036 -
Attachment is obsolete: true
Attachment #8485046 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8484436 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 46•10 years ago
|
||
Attachment #8484437 -
Attachment is obsolete: true
Attachment #8485053 -
Flags: review?(bzbarsky)
Comment 47•10 years ago
|
||
Comment on attachment 8484433 [details] [diff] [review] Part 1 - make all Constructor methods return already_AddRefed<...> v1 r=me
Attachment #8484433 -
Flags: review?(bzbarsky) → review+
Comment 48•10 years ago
|
||
Comment on attachment 8484434 [details] [diff] [review] Part 2 - remove instances of non-refcounted sequence return type v1 >+// Disabled until we figure out how to use return values to pass nsTArrays Just leave them enabled but change the TestBindingHeader.h bits, please?
Attachment #8484434 -
Flags: review?(bzbarsky) → review+
Comment 49•10 years ago
|
||
Comment on attachment 8484436 [details] [diff] [review] Part 4 - statically assert that we keep a strong reference to return values of NewObject methods v1 >+// Work around a bug in Visual Studio 2010 and 2012 This is identical to the code in TestMaybe.cpp. Should it live in some shared spot, perhaps? r=me
Attachment #8484436 -
Flags: review?(bzbarsky) → review+
Comment 50•10 years ago
|
||
Comment on attachment 8485046 [details] [diff] [review] Part 3 - use overloaded functions and auto to select the right type to store return values v1.1 >+ return nsRefPtr<T>(Move(aPtr)); I think I'd somewhat prefer the more explicit: return Move(nsRefPtr<T>(Move(aPtr))); here. >+ result = CGGeneric("auto") Should this be auto&? Or do we need to Move() the retval of StrongOrRawPtr? Or are we guaranteed that if a function returns nsRefPtr<T> and we pass that function's retval as a ctor arg to nsRefPtr<T> we'll end up invoking the move ctor? I really hate relying on implicit stuff. :( > if returnType.isCallback(): If we wanted to, we could do the same thing for those.... Followup is fine. r=me assuming we really are guaranteed there are no extra addrefs/releases here
Attachment #8485046 -
Flags: review?(bzbarsky) → review+
Comment 51•10 years ago
|
||
Comment on attachment 8485053 [details] [diff] [review] Part 5 - remove remaining traces of resultNotAddRefed v1.1 r=me. This is lovely. Please update the MDN docs too?
Attachment #8485053 -
Flags: review?(bzbarsky) → review+
Comment 52•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #50) > >+ return nsRefPtr<T>(Move(aPtr)); > > I think I'd somewhat prefer the more explicit: > > return Move(nsRefPtr<T>(Move(aPtr))); > > here. You're calling a constructor and returning an object, and that object is clearly a temporary. Move() isn't necessary, and I don't understand the value in having it.
Comment 53•10 years ago
|
||
> Move() isn't necessary Understanding that requires knowing a nontrivial amount of C++ knowledge. > and I don't understand the value in having it Making it crystal clear that the value is being moved.
Assignee | ||
Comment 54•10 years ago
|
||
Discussed with bz, I'm going to use |return aPtr.template downcast<T>();| instead.
Assignee | ||
Comment 55•10 years ago
|
||
Attachment #8486668 -
Flags: review?(jwalden+bmo)
Comment 56•10 years ago
|
||
Comment on attachment 8486668 [details] [diff] [review] Part 4a - rename the DECLTYPE macro and move it into its own header v1 Review of attachment 8486668 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/DecltypeMemberType.h @@ +4,5 @@ > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef mozilla_DecltypeMemberType_h > +#define mozilla_DecltypeMemberType_h > + You'll need to #include "mozilla/Compiler.h" for this head to work. Add a quick mfbt/tests/TestDecltypeMemberType.cpp file that trivially exercises this, to be sure it works with all compilers. @@ +13,5 @@ > +#if MOZ_IS_MSVC > +# if MOZ_MSVC_VERSION_AT_LEAST(12) > +# define MOZ_DECLTYPE(EXPR) decltype(EXPR) > +# else > + template<typename T> struct Identity { typedef T type; }; If we're defining a template/symbol here, please enclose it in namespace mozilla and namespace detail. Maybe just put the whole compiler-#if within both blocks, actually, so you don't have to duplicate it for this instance *and* for the gcc instance of it.
Attachment #8486668 -
Flags: review?(jwalden+bmo) → feedback+
Assignee | ||
Comment 57•10 years ago
|
||
Comment on attachment 8486668 [details] [diff] [review] Part 4a - rename the DECLTYPE macro and move it into its own header v1 It turns out I actually don't need this macro, it was leftover from an earlier attempt at fixing this. I removed its use from part 4.
Attachment #8486668 -
Attachment is obsolete: true
Assignee | ||
Comment 58•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc484f4f6a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce96b8dd847 https://hg.mozilla.org/integration/mozilla-inbound/rev/c848479c0127 https://hg.mozilla.org/integration/mozilla-inbound/rev/65a8e5b6c2ee https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0d430f66f3 I'll update MDN docs next.
Comment 59•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cfc484f4f6a4 https://hg.mozilla.org/mozilla-central/rev/2ce96b8dd847 https://hg.mozilla.org/mozilla-central/rev/c848479c0127 https://hg.mozilla.org/mozilla-central/rev/65a8e5b6c2ee https://hg.mozilla.org/mozilla-central/rev/9e0d430f66f3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 61•10 years ago
|
||
I'd written something but wasn't entirely happy with it. I've done this: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings$compare?to=669833&from=660599 Feel free to improve it.
Flags: needinfo?(peterv)
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•