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

RESOLVED FIXED in mozilla35

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: smaug, Assigned: peterv)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 13 obsolete attachments)

8.20 KB, patch
Details | Diff | Splinter Review
9.28 KB, patch
Details | Diff | Splinter Review
6.25 KB, patch
Details | Diff | Splinter Review
13.95 KB, patch
Details | Diff | Splinter Review
34.07 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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

5 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

5 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

5 years ago
Created attachment 723775 [details] [diff] [review]
crashing WIP
(Reporter)

Comment 4

5 years ago
I guess my SFINAE is still wrong.
(Reporter)

Comment 6

5 years ago
Because we get for example StrongOrRawPtr<JSObject> and JSObject certainly doesn't have
AddRef/Release methods.
(Reporter)

Comment 7

5 years ago
Hmm, but now that you ask... why do we generate such code.
(Reporter)

Comment 8

5 years ago
It is because of workers.
(Reporter)

Comment 9

5 years ago
Created attachment 723896 [details] [diff] [review]
patch

This might work
https://tbpl.mozilla.org/?tree=Try&rev=c35c6d5ea771
Attachment #723775 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
Tryserver totally does not like the patch
(Reporter)

Comment 11

5 years ago
because it is missing a null check. ugh.
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

5 years ago
Created attachment 723981 [details] [diff] [review]
patch

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

5 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.
I'm not sure I follow comment 15.  What exactly is the proposal?
(Reporter)

Updated

5 years ago
Attachment #723981 - Flags: review?(bzbarsky)
(Reporter)

Comment 17

5 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;
}
Ah, I see.  Hmm.  That might work pretty well for getters; doing it for methods might be a bit more of a pain.  Maybe.
I think we should just do the branch and pay whatever the tiny perf hit is.  The usability benefits are worth it.
Smaug, do you mind if I finish this?
Flags: needinfo?(bugs)
(Reporter)

Comment 21

5 years ago
Feel free to take this, but I don't think we should take the branch approach.
Flags: needinfo?(bugs)
Why not?  The measured hit was pretty small, and the maintainability benefits are pretty nice...
Created attachment 744385 [details] [diff] [review]
Updated patch
Attachment #723981 - Attachment is obsolete: true
Attachment #744385 - Flags: review?(bzbarsky)
(Reporter)

Comment 24

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

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)
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-
Created 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

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)
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.
Created attachment 751988 [details] [diff] [review]
part 3.  Remove vestiges of resultNotAddRefed.
Attachment #751988 - Flags: review?(bugs)
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

5 years ago
Attachment #751988 - Flags: review?(bugs) → review+
(Reporter)

Comment 32

5 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+
Assignee: bzbarsky → nobody
Whiteboard: [need review]
(Assignee)

Comment 33

4 years ago
Created attachment 8484433 [details] [diff] [review]
Part 1 - make all Constructor methods return already_AddRefed<...> v1
Assignee: nobody → peterv
Status: NEW → ASSIGNED
(Assignee)

Comment 34

4 years ago
Created attachment 8484434 [details] [diff] [review]
Part 2 - remove instances of non-refcounted sequence return type v1
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

4 years ago
Created attachment 8484435 [details] [diff] [review]
Part 3 - use templatized class to hold strong or weak pointer depending on the return type v1
(Assignee)

Comment 36

4 years ago
Created attachment 8484436 [details] [diff] [review]
Part 4 - statically assert that we keep a strong reference to return values of NewObject methods v1
(Assignee)

Comment 37

4 years ago
Created attachment 8484437 [details] [diff] [review]
Part 5 - remove remaining traces of resultNotAddRefed.
(Assignee)

Comment 38

4 years ago
Created attachment 8484438 [details] [diff] [review]
Part 3 - use overloaded functions and auto to select the right type to store return values v1
Attachment #8484435 - Attachment is obsolete: true
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

4 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

4 years ago
BTW, the codegen explicitly specifies the value of T, so that we can avoid ending up with totally unrelated types of objects.
> 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

4 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

4 years ago
Created attachment 8485036 [details] [diff] [review]
Part 3 - use overloaded functions and auto to select the right type to store return values v1.1
Attachment #8484438 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8484433 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8484434 - Flags: review?(bzbarsky)
(Assignee)

Comment 45

4 years ago
Created attachment 8485046 [details] [diff] [review]
Part 3 - use overloaded functions and auto to select the right type to store return values v1.1
Attachment #8485036 - Attachment is obsolete: true
Attachment #8485046 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Attachment #8484436 - Flags: review?(bzbarsky)
(Assignee)

Comment 46

4 years ago
Created attachment 8485053 [details] [diff] [review]
Part 5 - remove remaining traces of resultNotAddRefed v1.1
Attachment #8484437 - Attachment is obsolete: true
Attachment #8485053 - Flags: review?(bzbarsky)
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 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 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 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 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+
(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.
> 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

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

Comment 55

4 years ago
Created attachment 8486668 [details] [diff] [review]
Part 4a - rename the DECLTYPE macro and move it into its own header v1
Attachment #8486668 - Flags: review?(jwalden+bmo)
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

4 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
Peter, do you want me to just update the docs?
Flags: needinfo?(peterv)
(Assignee)

Comment 61

4 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)
You need to log in before you can comment on or make changes to this bug.