Improve API support for self-hosted functions

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: wingo, Assigned: wingo)

Tracking

unspecified
mozilla27
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Assignee: nobody → 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.
Attachment #809779 - Flags: review?(till)
Attachment #809779 - Flags: review?(jorendorff)
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.
Attachment #809779 - Flags: review?(till) → review+
Blocks: 907077
Attachment #809779 - Attachment is obsolete: true
Attachment #809779 - Flags: review?(jorendorff)
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.
Attachment #809911 - Attachment description: Improve API support for self-hosted functions → Improve API support for self-hosted functions v2
Attachment #809911 - Flags: review?(bzbarsky)
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
Great work here.
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.
Attachment #809911 - Flags: review?(bzbarsky) → review+
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
Attachment #809911 - Attachment is obsolete: true
Attachment #810443 - Flags: review+
Build fix for BindingUtils changes due to review; tried here: https://tbpl.mozilla.org/?tree=Try&rev=7965f16c90bf

r=till, r=bz
Attachment #810443 - Attachment is obsolete: true
Attachment #810529 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f46fd6fcf018
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.