773546 - Change Codegen to centralize specializing top-half for DOM methods and accessors

Status: RESOLVED FIXED

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

Description

[Eric Faust [:efaust]]

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=747290#c2 , we want to move to a centralizing top-half that unwraps and checks for proper DOM interface chaining of the |this| object before specializing to the bottom half to be stored in a slot on the JSFunction.

Comment 1

[Eric Faust [:efaust]]

At what point do we intend to put the bottom halves into slots on the objects? There doesn't seem to be a convenient time to do so. They are passed in and wrapped by JS_DefineProperties() and JS_DefineFunctions(), and then we do not have clean access to the JSFunctions. Boris, did you have some insight on this that I am missing?

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we have two options here, I think. At least if I understand the JS engine setup for this stuff....

Option 1 is to stop using JS_DefineProperties and JS_DefineFunctions and instead basically reimplement that stuff ourselves, but using function objects with reserved slots that we then stash things in.

Option 2 is to rely on the vp[0] in the top half being the callee function, which had better be our DOM function, right?  So it seems like we could just poke at its native to get our bottom half...  That assumes there's no weirdness with function proxies and so forth, though, which is something I don't know much about.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and I guess option 1 _still_ relies on getting the function from vp[0], doesn't it?

Comment 4

[Eric Faust [:efaust]]

Yes, but won't the native in the JSFunction callee always be the centralized top-half native? The whole point was to have the specialized function in a reserved slot, right? Or am I misunderstanding?

Either way, by the time we get to the centralized native, it's way too late to be thinking about how to get the specialized native into a slot. Indeed I expect the top-half to rely upon unpacking a slot from the callee it finds in vp[0], but that doesn't save us from our definition problems.

If we are going to not use JS_DefineProperties(), I should probably also undo the changes I made to it, or better yet not land those changes at all, and just do this first.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the way I was thinking of this is that we would generate a property spec that has the following information in it:

  { general_getter, specialized_getter_for_this_prop }

The general_getter function pointer would be the same for all the props on an interface.  The specialized_getter functions would all be different for the different props.

I'd been thinking about storing the specialized_getter in a slot, but there's no need for that: the JSFunction just has a pointer to it already, right?

Does that make sense?

Comment 6

[Eric Faust [:efaust]]

So in the new scheme JSJitInfo is a misnomer, as it's really more like "super secret dom bottom-half stash that's like a slot, but not really because it has more data in it"? ;)

So we get the callee, ask it for its jitinfo, and then use the bottom half from there. And we don't have addition problems either, since we will have already modified JS_DefineProperties() and JS_DefineFunctions() to accept JitInfo structs.

That's much much simpler :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

That was the thinking, yes.  ;)

Comment 8

[Eric Faust [:efaust]]

Attachment: Punch a hole for Binding Codgen to get JSJitInfos back

applies on top of Codegen patch in bug 747287

Comment 9

[Eric Faust [:efaust]]

Attachment: Patch

Applies on top of patch in bug 775289.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 644081 [details] [diff] [review]
Punch a hole for Binding Codgen to get JSJitInfos back

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

::: js/src/jsfriendapi.h
@@ +313,5 @@
>  
> +struct Function {
> +    Object base;
> +    uint16_t nargs;
> +    uint16_t flahs;

flahs?

@@ +1249,5 @@
>      bool isConstant;      /* Getting a construction-time constant? */
>  };
> +
> +static JS_ALWAYS_INLINE const JSJitInfo *
> +JSVAL_TO_JITINFO(jsval v)

const Value&

@@ +1251,5 @@
> +
> +static JS_ALWAYS_INLINE const JSJitInfo *
> +JSVAL_TO_JITINFO(jsval v)
> +{
> +    return reinterpret_cast<js::shadow::Function *>(JSVAL_TO_OBJECT(v))->jitinfo;

&v.toObject()

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 644081 [details] [diff] [review]
Punch a hole for Binding Codgen to get JSJitInfos back

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

::: js/src/jsfriendapi.h
@@ +1251,5 @@
> +
> +static JS_ALWAYS_INLINE const JSJitInfo *
> +JSVAL_TO_JITINFO(jsval v)
> +{
> +    return reinterpret_cast<js::shadow::Function *>(JSVAL_TO_OBJECT(v))->jitinfo;

How about naming it FUNCTION_VALUE_TO_JITINFO and adding
  JS_ASSERT(JS_GetClass(&v.toObject()) == &FunctionClass)
?

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 644083 [details] [diff] [review]
Patch

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

::: dom/bindings/Codegen.py
@@ +960,5 @@
>                  return "JSPROP_READONLY | " + flags
>              return flags
>  
>          def getter(attr):
> +            return ("{(JSPropertyOp)genericGetter, &%(name)s_getterinfo}" 

Drop trailing whitespace.

Comment 13

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f97d1a956d56
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e17c9f2e4a

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f97d1a956d56
https://hg.mozilla.org/mozilla-central/rev/83e17c9f2e4a