849567 - Automatically handle raw and already_AddRefed return values in Paris bindings and remove resultNotAddRefed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Needs some macro magic to make it work, but hopefully patch coming.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: crashing WIP

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I guess my SFINAE is still wrong.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Why do we need SFINAE here at all?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Because we get for example StrongOrRawPtr<JSObject> and JSObject certainly doesn't have
AddRef/Release methods.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Hmm, but now that you ask... why do we generate such code.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

It is because of workers.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This might work
https://tbpl.mozilla.org/?tree=Try&rev=c35c6d5ea771

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Tryserver totally does not like the patch

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

because it is missing a null check. ugh.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=4f40648c9071

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=86bba39f175c

The previous patch worked, so perhaps this one passes tests too.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow comment 15.  What exactly is the proposal?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

I think we should just do the branch and pay whatever the tiny perf hit is.  The usability benefits are worth it.

Comment 20

[David Zbarsky (:dzbarsky)]

Smaug, do you mind if I finish this?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Feel free to take this, but I don't think we should take the branch approach.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Why not?  The measured hit was pretty small, and the maintainability benefits are pretty nice...

Comment 23

[David Zbarsky (:dzbarsky)]

Attachment: Updated patch

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

pass and already_AddRefed to one of the wrap methods and end up just
calling get() on it and leaking.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744385 [details] [diff] [review]
Updated patch

Let's see if we can avoid the perf hit.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop using alreadyAddReffed information in codegen. just pass the return value of our C++ call directly to a

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove vestiges of resultNotAddRefed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 33

[Peter Van der Beken [:peterv]]

Attachment: Part 1 - make all Constructor methods return already_AddRefed<...> v1

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: Part 2 - remove instances of non-refcounted sequence return type v1

Comment 35

[Peter Van der Beken [:peterv]]

Attachment: Part 3 - use templatized class to hold strong or weak pointer depending on the return type v1

Comment 36

[Peter Van der Beken [:peterv]]

Attachment: Part 4 - statically assert that we keep a strong reference to return values of NewObject methods v1

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: Part 5 - remove remaining traces of resultNotAddRefed.

Comment 38

[Peter Van der Beken [:peterv]]

Attachment: Part 3 - use overloaded functions and auto to select the right type to store return values v1

Comment 39

[Boris Zbarsky [:bzbarsky]]

StrongOrRawPtr should probably have an overload taking nsRefPtr too, right?  Otherwise functions that currently return one will get the raw-pointer thing...

Comment 40

[Peter Van der Beken [:peterv]]

(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.

Comment 41

[Peter Van der Beken [:peterv]]

BTW, the codegen explicitly specifies the value of T, so that we can avoid ending up with totally unrelated types of objects.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 43

[Peter Van der Beken [:peterv]]

(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.

Comment 44

[Peter Van der Beken [:peterv]]

Attachment: Part 3 - use overloaded functions and auto to select the right type to store return values v1.1

Comment 45

[Peter Van der Beken [:peterv]]

Attachment: Part 3 - use overloaded functions and auto to select the right type to store return values v1.1

Comment 46

[Peter Van der Beken [:peterv]]

Attachment: Part 5 - remove remaining traces of resultNotAddRefed v1.1

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484433 [details] [diff] [review]
Part 1 - make all Constructor methods return already_AddRefed<...> v1

r=me

Comment 48

[Boris Zbarsky [:bzbarsky]]

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?

Comment 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[Boris Zbarsky [:bzbarsky]]

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

Comment 51

[Boris Zbarsky [:bzbarsky]]

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?

Comment 52

[Jeff Walden [:Waldo]]

(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

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 54

[Peter Van der Beken [:peterv]]

Discussed with bz, I'm going to use |return aPtr.template downcast<T>();| instead.

Comment 55

[Peter Van der Beken [:peterv]]

Attachment: Part 4a - rename the DECLTYPE macro and move it into its own header v1

Comment 56

[Jeff Walden [:Waldo]]

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.

Comment 57

[Peter Van der Beken [:peterv]]

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.

Comment 58

[Peter Van der Beken [:peterv]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 60

[Boris Zbarsky [:bzbarsky]]

Peter, do you want me to just update the docs?

Comment 61

[Peter Van der Beken [:peterv]]

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.