534733 - add support for fully custom quickstubs

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: teach qsgen.py about custom quickstubs

There are a few places in the code where we really want fully custom quickstubs, so that we can support overloaded methods that we can't express with XPIDL.  This is just the qsgen portion of this, the two consumers of this initially will be in canvas 2d & webgl.

The general approach is:

- in the qsconf, tell qsgen to not actually generate code for a particular quickstub, but assume it's present
- add a file to the custom quickstubs list which implements custom code for that quickstub

to support overloads:

- in the IDL, keep the method name as a dummy, e.g.:

void someMethod();

- then add multiple noscript variants, one for each overload:

[noscript] void someMethod_foo(in nsIFoo aFoo);
[noscript] void someMethod_value(in long aValue);

- in the custom quickstub, examine the JS types and do the appropriate conversions to call either _foo or _value.

Comment 1

[Jason Orendorff [:jorendorff]]

It's hard to review this without an example of its use. For example, are we going to quickstub both someMethod_foo and someMethod_value, and have the custom code for someMethod call those, or is the plan to have the custom code for someMethod do the unboxing and boxing itself? Surely the latter; but then why do we need the change to qsgen's treatment of [noscript]?

Why do we need customReturnInterfaces? Can't the XPIDL say what we mean?

    nsITheActualReturnType someMethod();

In general I wish we could make XPIDL more expressive, and update qsgen.py to support a new feature or two, instead of this change. It seems like this will facilitate some amount of cruft that we'll be stuck with forever.

On to nits:

In addStubMember, it looks like you added an `if` so as not to waste time. But the code is intended to check all the parameters for magic attributes that qsgen doesn't understand, for robustness.

> cmc.has_key('skipgen') and cmc['skipgen']

You can just write `cmc.get('skipgen', False)`. The same nit applies in one other place. (has_key is obsolete anyway; you can write `'skipgen' in cmc`.)

Comment 2

[Jason Orendorff [:jorendorff]]

One reason to actually add support for overloaded methods to XPIDL is that we can generate two traceable natives for the same JSFastNative, one for each signature.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 417561 [details] [diff] [review]
teach qsgen.py about custom quickstubs

Clearing the review bit for now. I want to push back a little on the approach, but if it's the right thing (or it can't be helped) I can re-review once I have a better grasp of how this is supposed to work.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

You can take a look at the two dependent bugs for how this is intended to be used.

The noscript bit is needed because xpidl requires any methods that use native type pointers to be marked [noscript], which is fine, but in this case I want to generate quickstubs for them because they are actually scriptable, just using a custom approach.  I could avoid this by having two methods in the IDL, such as:

void uniform1fv ([optional] in int dummy);
[noscript] void uniform1fv_array (in int location, in WebGLArrayPtr data);

instead of the current:

[noscript] void uniform1fv (in int location, in WebGLArrayPtr data);

To make this clearer in qsgen, I can introduce a separate list of methods for which custom stubs should be generated -- regardless of how they're listed in the IDL (noscript etc.).

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #1)
> Why do we need customReturnInterfaces? Can't the XPIDL say what we mean?
> 
>     nsITheActualReturnType someMethod();

Oh, forgot to respond to this -- the problem is that the method can return one of 2-3 types.  It's the only method that returns these types that's quickstubbed, and so the needed code isn't generated for those methods at all since they're never referenced as a return type.  I can add dummy methods with the right return types, but putting that in the IDL seems silly.

I agree that the solution to all of this is to teach xpidl etc. about method overloads, but that's not a small project, and I don't think doing that should block getting this stuff in -- both of this are huge perf wins on canvas 2D and webgl-related code.  (In particular, the 2D case is really wanted by the fennec folks.)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated, v2

Updated with the two nits addressed, but same basic approach.

I can avoid the noscript handling change if you're required to do:

  foo();
  [noscript] foo_real(....);

even when there is just one foo and there aren't any overloads in play; if you'd prefer that form, I can make the change.

I don't think there's a way around the return value issue, without either putting the list here or putting in dummy functions into the IDL.  Or maybe explicitly listing those interfaces in the interface list, and ensuring that all interfaces listed as such can be used as return types?

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 420203 [details] [diff] [review]
updated, v2

Hmm. Now that I've looked a little at the use cases, I don't see any quick way around this either.

Comment 8

[Peter Van der Beken [:peterv]]

This is pretty ugly, any change to quickstubs is now going to require changes to the boilerplate code in all those non-generated methods :-/.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/8d3d6f1dfd6a