920433 - Improve API support for self-hosted functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

The attached patch adds a few things:

  * A new JS_FNJS define, for defining self-hosted JSFunctionSpecs, and changes existing raw-JSFunctionSpec-struct users to use it;

  * A new JS_FNSPEC define, like JS_FNINFO but with a selfHostedFunction arg, and change Codegen.py to emit it (a later patch makes Codegen.py emit self-hosted @@iterator definitions);

  * A "selfHostedName" argument to JS_NewFunctionById, and corresponding updates to its only caller in dom/bindings/BindingUtils.cpp;

  * changes to lazy self-hosted functions to store their selfHostedName as an atom in the private field instead of a static reference to the functionspec.  This is needed for the JS_NewFunctionById API change.  It shouldn't add to the memory footprint as those atoms are needed anyway by the selfHostingGlobal.  Besides being nasty, storing the raw const char* is not an option as that pointer can have a low bit of 1.

Comment 1

[Andy Wingo [:wingo]]

Attachment: Improve API support for self-hosted functions

Comment 2

[Andy Wingo [:wingo]]

Comment on attachment 809779 [details] [diff] [review]
Improve API support for self-hosted functions

Also, JS_DefineProperties now respects the flags argument, and some incorrect self-hosted flags uses were fixed.

Comment 3

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=a2c8bab7f98f FWIW

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 809779 [details] [diff] [review]
Improve API support for self-hosted functions

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

Very nice, this is all much cleaner!

jorendorff, I don't think you need to look through all this. You might want to look at the jsapi.h changes, as you might disagree with my assessment that introducing a new JS:: function is preferable.

wingo, please r? bz for the BindingUtils and Codegen changes on the next version of the patch. For the rest, you can carry my r+.

::: js/src/builtin/ParallelArray.cpp
@@ +37,5 @@
>      // specialized version of get() based on the dimensionality of the
>      // receiver.  In the future we can improve this by (1) extending
>      // TI to track the dimensionality of the receiver and (2) using a
>      // hint to aggressively inline calls to get().
>      // { "get",      JSOP_NULLWRAPPER, 1, 0, "ParallelArrayGet" },

Can you change this one, too? It might get activated at one point, but even if not, it's documentation that now makes less sense.

::: js/src/jsapi.cpp
@@ +4028,5 @@
>  }
>  
>  JS_PUBLIC_API(JSFunction *)
>  JS_NewFunctionById(JSContext *cx, JSNative native, unsigned nargs, unsigned flags, JSObject *parentArg,
> +                   jsid id, const char *selfHostedName)

Let's do
JS::GetSelfHostedFunction(JSContext *cx, jsid id, const char *selfHostedName, unsigned nargs);

The duplication consists of 4 asserts and the atom-rooting, which seems fair.

@@ +4262,2 @@
>                  return false;
> +            if (!JSObject::defineGeneric(cx, obj, id, funVal, NULL, NULL, flags))

Before the recent refactoring in bug 918823, this all looked pretty different. In the end, though, the flags where ignored, too. For no additional information whatsoever, see bug 918823, comment 6.

If this works and doesn't regress any tests, I'm totally for not arbitrarily ignoring the flags.

::: js/src/jsapi.h
@@ +2396,3 @@
>  #define JS_FNINFO(name,call,info,nargs,flags)                                 \
> +    JS_FNSPEC(name, call, info, nargs, flags, nullptr)
> +#define JS_FNJS(name,selfHostedName,nargs,flags)                              \

I think I'd prefer something less generic. These aren't any old JS functions, after all. Either JS_FNSH, or maybe even JS_SHFN. Or something less unspeakable, such as JS_SELFHOSTED_FN, even? I don't know how great my taste in macro naming is, so I'm hesitant to demand the latter.

::: js/src/jsarray.cpp
@@ +2824,4 @@
>      JS_FN("filter",             array_filter,       1,JSFUN_GENERIC_NATIVE),
> +    JS_FNJS("some",             "ArraySome",        1,0),
> +    JS_FNJS("every",            "ArrayEvery",       1,0),
> +    

Nit: whitespace

::: js/src/vm/GlobalObject.h
@@ +457,5 @@
>          return JSObject::setProperty(cx, holder, holder, name, &valCopy, false);
>      }
> +
> +    bool getSelfHostedFunction(JSContext *cx, HandleAtom name, unsigned nargs,
> +                               HandleAtom selfHostedName, MutableHandleValue funVal);

Can you change the order to cx, selfHostedName, name, nargs? Seems clearer to me.

Comment 5

[Andy Wingo [:wingo]]

Attachment: Improve API support for self-hosted functions v2

Comment 6

[Andy Wingo [:wingo]]

Comment on attachment 809911 [details] [diff] [review]
Improve API support for self-hosted functions v2

r=? to bz for BindingUtils.cpp / Codegen.py.  Thanks :-)
r=till for the rest.

Comment 7

[Tom S]

Comment on attachment 809911 [details] [diff] [review]
Improve API support for self-hosted functions v2

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

::: js/src/jsapi.cpp
@@ +4052,5 @@
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +
> +    RootedAtom name(cx, JSID_TO_ATOM(id));
> +    RootedAtom shName(cx, Atomize(cx, selfHostedName, strlen(selfHostedName)));

missing null check

Comment 8

[Jason Orendorff [:jorendorff]]

Great work here.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 809911 [details] [diff] [review]
Improve API support for self-hosted functions v2

>+++ b/dom/bindings/BindingUtils.cpp
>             SET_JITINFO(fun, methodSpec.call.info);

We should be asserting that methodSpec.call.info is null if methodSpec.selfHostedName, I would think.  Seems bad if anything else ever happens.

r=me with that.

Comment 10

[Andy Wingo [:wingo]]

Attachment: Improve API support for self-hosted functions v3

Fixed shName null check.  (AFAIU the id->atom didn't need one, as it must be a string already.)

I fixed the SET_JITINFO bit as well.  In fact a future patch that actually defines self-hosted functions through the binding generator hit this case; thankfully the debug-mode assert in SET_JITINFO that the function was not interpreted caught this in an overnight try build.  Now SET_JITTINFO is only called if the function is not self-hosted.

carrying r=till, r=bz

Comment 11

[Andy Wingo [:wingo]]

Attachment: Improve API support for self-hosted functions v4

Build fix for BindingUtils changes due to review; tried here: https://tbpl.mozilla.org/?tree=Try&rev=7965f16c90bf

r=till, r=bz

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f46fd6fcf018

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f46fd6fcf018