Last Comment Bug 773546 - Change Codegen to centralize specializing top-half for DOM methods and accessors
: Change Codegen to centralize specializing top-half for DOM methods and accessors
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Eric Faust [:efaust]
:
Mentors:
Depends on: 773846 781040
Blocks: 747290 773548
  Show dependency treegraph
 
Reported: 2012-07-12 23:48 PDT by Eric Faust [:efaust]
Modified: 2012-08-08 09:27 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Punch a hole for Binding Codgen to get JSJitInfos back (2.68 KB, patch)
2012-07-19 17:27 PDT, Eric Faust [:efaust]
luke: review+
Details | Diff | Splinter Review
Patch (12.53 KB, patch)
2012-07-19 17:29 PDT, Eric Faust [:efaust]
peterv: review+
Details | Diff | Splinter Review

Description Eric Faust [:efaust] 2012-07-12 23:48:46 PDT
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] 2012-07-13 00:10:05 PDT
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 [:bz] 2012-07-13 02:07:27 PDT
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 [:bz] 2012-07-13 02:07:58 PDT
Oh, and I guess option 1 _still_ relies on getting the function from vp[0], doesn't it?
Comment 4 Eric Faust [:efaust] 2012-07-13 02:28:22 PDT
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 [:bz] 2012-07-13 02:32:15 PDT
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] 2012-07-13 02:51:06 PDT
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 [:bz] 2012-07-13 08:26:48 PDT
That was the thinking, yes.  ;)
Comment 8 Eric Faust [:efaust] 2012-07-19 17:27:25 PDT
Created attachment 644081 [details] [diff] [review]
Punch a hole for Binding Codgen to get JSJitInfos back

applies on top of Codegen patch in bug 747287
Comment 9 Eric Faust [:efaust] 2012-07-19 17:29:32 PDT
Created attachment 644083 [details] [diff] [review]
Patch

Applies on top of patch in bug 775289.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-07-20 03:10:19 PDT
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] 2012-07-20 03:18:19 PDT
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] - away till Aug 1st 2012-08-02 03:06:35 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.