766448 - Reflow PropertySpec and JS_DefineProperties to accept additional DOM metadata field.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Property Specs need to contain an additional field, to be NULL in all non-DOM cases, and set in the DOM case for a piece of metadata to be stored in the JSFunction struct for DOM accessor methods.

To this end, we need to rework the JSFunction native half of the union, the accessors of that union, Property Specs themselves, and JS_DefineProperties.

Comment 1

[Eric Faust [:efaust]]

Attachment: Patch

Comment 2

[Nicolas B. Pierron [:nbp]]

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

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

::: js/src/jsapi.h
@@ +3873,5 @@
>      int8_t                tinyid;
>      uint8_t               flags;
>      JSPropertyOp          getter;
>      JSStrictPropertyOp    setter;
> +    JITinfo               *jitinfo;  /* NULL except for the DOM */

I am not sure to understand the goal of JITinfo, because you use it here and …

::: js/src/jsfun.h
@@ +53,5 @@
>      uint16_t        flags;        /* flags, see JSFUN_* below and in jsapi.h */
>      union U {
> +        struct Native {
> +            js::Native  native;       /* native method pointer or null */
> +            JITinfo     *jitinfo;     /* Information about this function to be

… here.

In one case your JITinfo describe both the getter and setter, and in the other case it describes only one native function.  Maybe it would be best to have 2 JITinfo for properties or change the JSPropertyOp to include a JITinfo with it.

Then I am not sure we are fine at breaking the backward compatibility of the API yet.  The best would be to provide constructors for JSPropertySpec to avoid that in the future.  That's why I was thinking at a separate data structure first but you should probably ask this on one of the mailing list.

nit: rename JITinfo to JITInfo.

Side notes:
(1) Cannot you keep the backward compatibility by using an implicit case from a function pointer to a JSPropertyOp class which contains a JITinfo pointer initialized to NULL by default?

(2) Nice extension of the JSFunction!  This sounds better than having an external mapping between JSNative to JITinfo.

::: js/src/shell/js.cpp
@@ +3914,4 @@
>      {"customNative",    ITS_CUSTOMNATIVE,
>                          JSPROP_ENUMERATE | JSPROP_NATIVE_ACCESSORS,
>                          (JSPropertyOp)its_get_customNative,
> +                        (JSStrictPropertyOp)its_set_customNative, NULL},

Take care with this modification, you will have to update the DOM at the same time.  In which case it would be better to land this patch in inbound instead of IonMonkey.

Before landing, you should ask if there is any issue on the mailing list, especially for people who are embedding Spider Monkey and which are likely to use these API.

Comment 3

[Eric Faust [:efaust]]

 (In reply to Nicolas B. Pierron [:pierron] from comment #2)
> In one case your JITinfo describe both the getter and setter, and in the
> other case it describes only one native function.  Maybe it would be best to
> have 2 JITinfo for properties or change the JSPropertyOp to include a
> JITinfo with it.

Our problem was that while the information should clearly be part of the JSPropertySpec, as it describes, as you point out, both the getter and the setter at once, and there is not a way to have it describe one without the other, there's not really another good place to keep track of it in the engine. It's not a thing that should be part of a more generic property descriptor, nor should it have any paths back to JS. We saw this place to sneak it in to describe the accessors of the DOM, and I chose to do that.

Comment 4

[Jeff Walden [:Waldo]]

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

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

Good except for style nits.  There are enough I probably should look at an updated patch to avoid failure to communicate.

The SpiderMonkey line length limit is 99 characters, so don't wrap unless you exceed that.  (The limit's lower, 79, for comment text, tho.)

::: js/src/jsapi.cpp
@@ +3694,5 @@
>      if (attrs & JSPROP_NATIVE_ACCESSORS) {
>          JS_ASSERT(!(attrs & (JSPROP_GETTER | JSPROP_SETTER)));
>          attrs &= ~JSPROP_NATIVE_ACCESSORS;
>          if (getter) {
>              JSObject *getobj = JS_NewFunction(cx, (Native) getter, 0, 0, &obj->global(), NULL);

If you change this to JSFunction*, I think you can avoid some casts below.  Likewise for the setter case.

@@ +3870,1 @@
>          return NULL;

if (!fun(...,
         ...))
{
    ....
}

is the correct style for this.

::: js/src/jsapi.h
@@ +3860,5 @@
>      uint8_t         flags;
>      uint8_t         spare[3];
>  };
>  
> +typedef struct JITinfo JITinfo;

This should be JSJitInfo, I think.  Or we could keep it named this way and put it in the JS namespace.  That's actually probably best if we can do it -- possibly jsd might put a crimp in that now, if it does just go with JSJitInfo.

::: js/src/jsfriendapi.h
@@ +1152,5 @@
>  JS_FRIEND_API(void *)
>  JS_GetDataViewData(JSObject *obj, JSContext *cx);
>  
> +/* Metadata passed from the DOM to the JS Engine for JIT optimizations on DOM
> + * property accessors */

Style (rewrap to the appropriate length, of course):

/*
 * Metadata passed from
 * the DOM to the ...
 * ...
 */

Also this should be formulated in complete sentences, properly punctuated.  I'd also add a note that in the long run we want to formulate this as something available to any JSAPI user, but that we'll need some experience with this and how it works before we're willing to sanction it in the fully public API.

::: js/src/jsfun.cpp
@@ +1284,5 @@
>          fun->mutableScript().init(NULL);
>          fun->initEnvironment(parent);
>      } else {
> +        fun->u.n.native = native;
> +        JS_ASSERT(fun->u.n.native);

Hm, while we're here, let's move this assert to the start of the block, to parallel the other one.

::: js/src/jsfun.h
@@ +51,5 @@
>      uint16_t        nargs;        /* maximum number of specified arguments,
>                                       reflected as f.length/f.arity */
>      uint16_t        flags;        /* flags, see JSFUN_* below and in jsapi.h */
>      union U {
> +        struct Native {

Make this class Native, then make JSFunction a friend.  (Or the accessor methods, but JSFunction's probably good enough.)

@@ +53,5 @@
>      uint16_t        flags;        /* flags, see JSFUN_* below and in jsapi.h */
>      union U {
> +        struct Native {
> +            js::Native  native;       /* native method pointer or null */
> +            JITinfo     *jitinfo;     /* Information about this function to be

This class is so much internal that I don't think we should worry at all about backward compatibility here.  At best there should be initNative and initScripted methods to do the assignments, but not much more than that.

If JITInfo is all compile-time constant data, we should make this field a const JITInfo* and update the accessors/setters accordingly.

Comment 5

[Eric Faust [:efaust]]

Attachment: Patch reflecting stylistic suggestions

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 636487 [details] [diff] [review]
Patch reflecting stylistic suggestions

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

::: js/src/jsapi.h
@@ +3852,5 @@
>      int8_t                tinyid;
>      uint8_t               flags;
>      JSPropertyOp          getter;
>      JSStrictPropertyOp    setter;
> +    const JSJitInfo       *jitinfo;  /* NULL except for the DOM */

Don't mention DOM here.  How about this instead, in more JSAPIish terms?

/* NULL unless the friend API is used for extra optimizations */

Comment 7

[Eric Faust [:efaust]]

Attachment: Patch with one JSJitInfo per function is JSPropertySpec

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 640877 [details] [diff] [review]
Patch with one JSJitInfo per function is JSPropertySpec

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

So much fugly.  :-(

::: js/src/jsapi.h
@@ +3842,5 @@
>      uint8_t         flags;
>      uint8_t         spare[3];
>  };
>  
> +typedef struct JSJitInfo JSJitInfo;

Hm, so I guess we have to have this type in the public API, in a way.  I guess that means we need a comment by it explaining that embedders shouldn't use it unless they're willing to put up with friend bustage, something like this:

/*
 * EMBEDDERS: We hope to make JIT info a public concept, but we're not
 *            prepared to write the current system in stone yet, not
 *            without gaining experience with it for awhile.  If you
 *            want to play with this fire, look to the friend API.
 *            Otherwise pass NULL to APIs asking for pointers of this
 *            type.
 */

@@ +3865,5 @@
> + * respectively, and make it easy to pass no JSJitInfo as part of a
> + * JSPropertySpec.
> + */
> +#define JSOPWRAPPER(op) {op, NULL}
> +#define JSOPNULLWRAPPER JSOPWRAPPER(NULL)

JSOP_WRAPPER and JSOP_NULLWRAPPER, I think.  This mashing-together is unreadable.

::: js/src/jsfriendapi.h
@@ +1175,5 @@
> + * optimizations on DOM property accessors. Eventually, this should be made
> + * available to general JSAPI users, but we are not currently ready to do so.
> + */
> +struct JSJitInfo;
> +

If we have to have this in JSAPI, this probably isn't necessary.

::: js/src/jsfuninlines.h
@@ +50,5 @@
> +    u.n.jitinfo = data;
> +}
> +
> +inline const JSJitInfo *
> +JSFunction::jitinfo() const

jitInfo

Comment 9

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46f6ff1007bc

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/46f6ff1007bc