Closed Bug 773546 Opened 8 years ago Closed 8 years ago

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


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

Not set





(Reporter: efaust, Assigned: efaust)


(Blocks 1 open bug)



(2 files)

As mentioned in , 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.
Depends on: 773547
Blocks: 773548
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?
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.
Oh, and I guess option 1 _still_ relies on getting the function from vp[0], doesn't it?
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.
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?
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 :)
That was the thinking, yes.  ;)
Depends on: 773846
No longer depends on: 773547
Assignee: nobody → efaust
applies on top of Codegen patch in bug 747287
Attachment #644081 - Flags: review?(luke)
Attached patch PatchSplinter Review
Applies on top of patch in bug 775289.
Attachment #644083 - Flags: review?(peterv)
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;


@@ +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;

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)
Attachment #644081 - Flags: review?(luke) → review+
Comment on attachment 644083 [details] [diff] [review]

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

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

Drop trailing whitespace.
Attachment #644083 - Flags: review?(peterv) → review+
Depends on: 781040
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.