1273661 - Add a cheaper codepath for IDL callbacks that don't end up being used by the callee

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Three changes:

1)  If we're not used, we never HoldJSObjects, which avoids all that work.
2)  If we're not used, we never get the incumbent global.
3)  If we're not used, we never get the stack (when stack saving is enabled).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, nevermind on #2 and #3.  I just realized that it fails if the callee calls right back into our callback.  :(

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to construct a callback object without calling HoldJSObjects

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a way to trace a RefPtr<T> or OwningNonNull<T> via a Rooted

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add the codegen bits and Trace implementation to allow Web IDL callbacks to not have to HoldJSObjects until the bindings have determined that someone is actually holding on to the callback object

Terrence, could you check the Trace method here in particular?

Comment 5

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

Comment on attachment 8753537 [details] [diff] [review]
part 1.  Add a way to construct a callback object without calling HoldJSObjects


>+  // Places that use this need to ensure that the callback is traced (e.g. via a
>+  // Rooted) until the HoldJSObjects call happens.
>+  struct FastCallbackConstructor {
>+  };
ok, this is one approach. Other would be perhaps to pass some enum to the ctor an based on the value call holdjs.
But perhaps this is safer.
Or could the public ctor call this protected ctor and explicitly call HoldJS? I guess that would be uglier since Init() is called in many places.
ok, this is fine.


>+
>+  // Just like the public version without the FastCallbackConstructor argument,
>+  // except for not calling HoldJSObjects.  If you use this, you MUST ensure
>+  // that the object is traced until the HoldJSObjects happens!
>+  CallbackObject(JSContext* aCx, JS::Handle<JSObject*> aCallback,
>+                 nsIGlobalObject *aIncumbentGlobal,
Nit, could be consistent with style. * goes with type.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> But perhaps this is safer.

Yeah, I wanted to make sure random consumers couldn't trigger the no-holding mode.

> Nit, could be consistent with style. * goes with type.

Will fix.  I copy/pasted existing code.  :(

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8753539 [details] [diff] [review]
part 2.  Add a way to trace a RefPtr<T> or OwningNonNull<T> via a Rooted

Review of attachment 8753539 [details] [diff] [review]:
-----------------------------------------------------------------

That looks correct to me.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8753541 [details] [diff] [review]
part 3.  Add the codegen bits and Trace implementation to allow Web IDL callbacks to not have to HoldJSObjects until the bindings have determined that someone is actually holding on to the callback object

Review of attachment 8753541 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem too unsavory, although it would be nice if you could get away with just Rooted<RefPtr<T>>. I guess perfect forwarding didn't work out?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I needed the nontrivial destructor here to do the real work.

But yeah, I couldn't get forwarding in the assignment operator to work...

Comment 10

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

Comment on attachment 8753539 [details] [diff] [review]
part 2.  Add a way to trace a RefPtr<T> or OwningNonNull<T> via a Rooted

rs+, this is all in terrence's world.

It is interesting that those operators work, even though they all just return self.get().  I guess some nested operator magic makes it all good.

Comment 11

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

Comment on attachment 8753541 [details] [diff] [review]
part 3.  Add the codegen bits and Trace implementation to allow Web IDL callbacks to not have to HoldJSObjects until the bindings have determined that someone is actually holding on to the callback object

>         # Do codegen for all the callback interfaces.  Skip worker callbacks.
>         cgthings.extend([CGCallbackInterface(x) for x in callbackDescriptors if
>                          not x.workers])
> 
>+        cgthings.extend([CGNamespace('binding_detail',
>+                                     CGFastCallback(x.interface))
>+                         for x in callbackDescriptors if not x.workers])
>+
I wish I understood why we're skipping worker callbacks for callback interfaces but not for callbacks.
Not really about this bug though.

>+        traceMethod = ClassMethod("Trace", "void",
>+                                  [Argument("JSTracer*", "aTracer")],
>+                                  inline=True,
>+                                  bodyInHeader=True,
>+                                  visibility="public",
>+                                  body="%s::Trace(aTracer);\n" % baseName)
>+        holdMethod = ClassMethod("HoldJSObjectsIfMoreThanOneOwner", "void",
>+                                 [],
>+                                 inline=True,
>+                                 bodyInHeader=True,
>+                                 visibility="public",
>+                                 body=(
>+                                     "%s::HoldJSObjectsIfMoreThanOneOwner();\n" %
>+                                     baseName))
I don't understand why we need these methods when the base class already has them. Just to make them public?


I wouldn't mind seeing generated code before and after this patch.

You tested the Optional<> case?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It is interesting that those operators work, even though they all just return self.get().

self.get() returns the type the Rooted is templated over.  Then that type itself may have implicit conversions to the actual return type of the operator (and does).

> I wish I understood why we're skipping worker callbacks for callback interfaces but not for callbacks.

I think just because https://hg.mozilla.org/mozilla-central/rev/3b16cca767c4 didn't add them.  We needed the worker-only callbacks for something, but not worker-only callback interfaces...

> I don't understand why we need these methods when the base class already has them. Just to make them public?

Yes.  The JS GC template machinery needs them public, or friend decls, and I didn't want random people calling it and putting in the right friend decls is a pain.  So I just used public methods forwarding to the protected ones.

> You tested the Optional<> case?

Kinda.  It failed to compile due to type mismatches (Optional<RootedCallback<OwningNonNull<Foo>>> being passed where the callee expected an Optional<OwningNonNull<Foo>>) in some of our bindings.  If we cared, we could make it work with some effort (basically provide a conversion operator on a specialization of Optional<RootedCallback> that returns a new Optional with the thing inside the RootedCallback or something), but I decided it wasn't worth it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Before and after codegen for addEventListener

Diff, then the full method before and the full method after.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Codegen for FastEventListener

Comment 15

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

(In reply to Boris Zbarsky [:bz] from comment #12)
> Yes.  The JS GC template machinery needs them public, or friend decls, and I
> didn't want random people calling it and putting in the right friend decls
> is a pain.  So I just used public methods forwarding to the protected ones.
ok

> Kinda.  It failed to compile due to type mismatches
> (Optional<RootedCallback<OwningNonNull<Foo>>> being passed where the callee
> expected an Optional<OwningNonNull<Foo>>) in some of our bindings. 
ok thanks.

> If we
> cared, we could make it work with some effort (basically provide a
> conversion operator on a specialization of Optional<RootedCallback> that
> returns a new Optional with the thing inside the RootedCallback or
> something), but I decided it wasn't worth it.
Yeah, probably not worth it.

thanks.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Addendum to part 2 to handle null OwningNonNull

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c08f417d85fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/9806d7a88802
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f319062be85

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74dbcb56456d

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb5b50fd858

Comment 20

[Boris Zbarsky [:bzbarsky]]

OK, I backed all that out.

I ran into two problems:

1)  We actually need different destructors for the RefPtr and OwningNonNull case, because they need to check for "never assigned" differently.

2)  Once I did that, MSVC refused to compile things for reasons I could not figure out; it kept thinking OwningNonNull is not a template or something.

I guess I get to have the fun of try-debugging on Windows.

Comment 21

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab87e09f57b

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Changes to the destructor to make things happy

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

I tried bdb5b50fd858 with VS2015 locally, and saw the identical issue you met with the infra. And I found out that prefixing OwningNonNull in the constructor with mozilla:: works around this failure. You may want to try that.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yeah, I figured out what's going on.  It was seeing the typedef from part 2 that lives up the ancestor chain.  I just removed the typedef and that made things happy.

Comment 25

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

Comment on attachment 8754459 [details] [diff] [review]
Changes to the destructor to make things happy

>+  template<typename U>
>+  static bool IsInitialized(U& aArg); // Not implemented
>+
>+  template<typename U>
>+  static bool IsInitialized(RefPtr<U>& aRefPtr)
>+  {
>+    return aRefPtr;
>+  }
>+
>+  template<typename U>
>+  static bool IsInitialized(OwningNonNull<U>& aOwningNonNull)
>+  {
>+    return aOwningNonNull.isInitialized();
>+  }
>+
Curious, could this stuff be private? Isn't it used only in dtor.


Wondering in which case IsInitialized returns false for OwningNonNull case?
I mean I would expect OwningNonNull to be always initialized before trying to pass the callback to
some (C++) method.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Curious, could this stuff be private? Isn't it used only in dtor.

Yes, it could.

> Wondering in which case IsInitialized returns false for OwningNonNull case?

Same as in <https://bug1273661.bmoattachments.org/attachment.cgi?id=8754083>.  I'll add a comment here pointing to that one.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1af1c31b07a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcaee6a9d587
https://hg.mozilla.org/integration/mozilla-inbound/rev/1287ce64f050

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a1af1c31b07a
https://hg.mozilla.org/mozilla-central/rev/bcaee6a9d587
https://hg.mozilla.org/mozilla-central/rev/1287ce64f050