458735 - Improve internal API for traceable natives

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: v0 (against mozilla-central)

The patch removes the knownNatives array in jstracer.cpp and many of the entries in builtins.tbl.  The information is still in JSTraceableNative and nanojit::CallInfo structs, but those are now declared alongside the native functions themselves.

So for example, with this patch Math.sin has
- a JSFastNative implementation
- a FASTCALL native implementation that operates on jsdoubles
- a CallInfo struct
- a JSTraceableNative struct
- an entry in a table of JSFunctionSpecs
all in the same file, jsmath.cpp.

The point of this change is to allow the embedding to declare traceable functions, something we want to do for the DOM.

As shaver suggested I used the clasp word in JSFunction to point to the JSTraceableNative, if any.

CallInfo tables are now set up using new macros defined in jsbuiltins.h.  The new macros make more sense to me; all the LO, LO, LO, F, P gibberish is gone.  However they are more preprocessor-intensive.

The patch changes jsint to int32 in a few places, just for consistency.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1 (against tracemonkey)

Comment 2

[Jason Orendorff [:jorendorff]]

Sorry for the crummy description.

The important new capability here is that instead of having a single, fixed, central table of builtins, we can now add builtins in places like XPConnect.  This is needed for tracing the DOM (bug 453738).

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 341930 [details] [diff] [review]
v1 (against tracemonkey)

>+        fun->u.n.u.clasp = clasp;

Could you use a C++ anonymous union, at leat to avoid the second "u."?

>+js_Array_p_push1(JSContext* cx, JSObject* obj, jsval v)
>+{
>+    if (!(OBJ_IS_DENSE_ARRAY(cx, obj) 
>+          ? js_array_push1_dense(cx, obj, v, &v)
>+          : js_array_push_slowly(cx, obj, 1, &v, &v))) {
>+        return JSVAL_ERROR_COOKIE;
>+    }
>+    return v;

This would not require PGO to reorder (although the pipe may contain the return v target too), and be a bit clearer, if you inverted the logic to lose the !.

>+js_Array_p_pop(JSContext* cx, JSObject* obj)
>+{
>+    jsval v;
>+    if (!(OBJ_IS_DENSE_ARRAY(cx, obj) 
>+          ? js_array_pop_dense(cx, obj, &v)
>+          : js_array_pop_slowly(cx, obj, &v))) {
>+        return JSVAL_ERROR_COOKIE;
>+    }
>+    return v;

Ditto, sorry if pre-existing, but since these are moving, may as well reorder.

>+JS_DEFINE_CALLINFO_3(STRING,    Array_p_join, CONTEXT, OBJECT, STRING,                      0, 0)

Is the cloned whitespace to column-align arguments worth keeping?

>+
>+static JSTraceableNative array_join_trcinfo[] = {
>+    { js_array_join,               &ci_Array_p_join,         "TC",  "s",    FAIL_NULL }};

Prevailing style wants that closing }; on the next line.

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

Addressing brendan's comments.

Comment 5

[Andreas Gal :gal]

I like the departure from fid-based builtin identification. Ed, what do you think?

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v3

This should break the browser build a bit less.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 341991 [details] [diff] [review]
v3

Brendan better to review this in a timely fashion, and Ed for the nanojit changes.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 341991 [details] [diff] [review]
v3

Applied this patch on x86-64 for kicks --

>         intN prefixc = strlen(known->prefix);
>         LIns* args[5];
>         LIns** argp = &args[argc + prefixc - 1];
>         char argtype;
Not related but GCC complained about jumps crossing initialization here,  

>         LIns* res_ins = lir->insCall(known->builtin, args);
and here, 

>+    next_specialization:
>+        if (!(known->flags & JSTN_MORE))
because of this jump target.

> 				case LIR_call:
> 				case LIR_fcall:
>-					i -= argwords(i->argc())+1;
>+					i -= argwords(i->argc())+2;
I think this needs to be 1+callInfoWords.

>-		_buf->commit(words+1);	
>+		_buf->commit(words+2);
Same as well.

Comment 9

[Edwin Smith]

Comment on attachment 341991 [details] [diff] [review]
v3

Burning 4bytes per call instead of 1 byte for the fid is a little hard to stomach, but I appreciate the niceness of splitting up the monolithic call table.  this also allows filters to introduce filter-specific calls w/out mixing concerns elsewhere (example: softfloat filter turning binary ops into calls for fadd, fsub, etc -- such a filter could be 100% vm-agnostic after this change).

at some point one of us will need >256 builtins, also motiviating this change.

at least one arg ref could be put into the byte that was vacated, net +3b.  maybe even less net growhth since args chew up 4b at a time (0,1,2,3 args require 4b, 4..7 args require 8b, etc).

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: v4

Changes in this version:
- Less dramatic build changes (change #includes instead of changing build system)
- Include dvander's fixes for 64-bit systems
- Update the big comment in builtins.tbl

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v5 - update to tip

Fix conflicts with bz's patch for |new Date|, which landed yesterday.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v6

Update to tip again, *and*:
- don't put C++ code into "friend-API" headers
- move some more array stuff from jsbuiltins to jsarray.cpp

The browser actually builds with this one.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 342142 [details] [diff] [review]
v6

>+enum JSTNErrType { INFALLIBLE, FAIL_NULL, FAIL_NEG, FAIL_VOID, FAIL_JSVAL };
>+enum { JSTN_ERRTYPE_MASK = 7, JSTN_MORE = 8 };

Nit: JSTN_ used by jsscript.h:JSTryNoteKind -- could just share the prefix, might consider JS_TF or JS_TN as the JSFunctionSpec "row initializer" macro instead of JS_TFN -- or use JSTFN_ as the prefix here, but that would want the type name to be JSTraceableFastNative (explicit, but overlong?).

>+/*
>+ * Declare a C function named js_<op> and a CallInfo struct named ci_<op> so
>+ * the tracer can call it.
>+ */
>+#define JS_DEFINE_CALLINFO_1(rt, op, at0, cse, fold)                                              \
>+    _JS_DEFINE_CALLINFO(op, _JS_CTYPE(rt), (_JS_CTYPE(at0)),                                      \
>+                        (_JS_ARGSIZE(at0) << 2) | _JS_RETSIZE(rt), cse, fold)

Elsewhere (in the .cpp file patches) the marching JS_DEFINE_CALLINFO* soldiers are followed by parallel trcinfo defs -- could these be combined profitably (sharing actual arguments, reducing repetition and unchecked coupling) with more macros?

>+++ b/js/src/jsdtracef.c
>@@ -49,18 +49,20 @@
> 
> #define TYPEOF(cx,v)    (JSVAL_IS_NULL(v) ? JSTYPE_NULL : JS_TypeOfValue(cx,v))
> 
> static char dempty[] = "<null>";
> 
> char *
> jsdtrace_funcclass_name(JSFunction *fun)
> {
>-    return (!FUN_INTERPRETED(fun) && fun->u.n.clasp)
>-           ? (char *)fun->u.n.clasp->name
>+    return (!FUN_INTERPRETED(fun) &&
>+            !(fun->flags & JSFUN_TRACEABLE) &&
>+            fun->u.n.u.clasp)
>+           ? (char *)fun->u.n.u.clasp->name
>            : dempty;

Holdover u.n.u. from before the anonymous union change to jsfun.h -- can you test DTRACE on before committing, or get someone at Sun to help?
Here is an example of the CI/TI repetition/coupling:

>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_sin,    DOUBLE,          1, 1)
>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_cos,    DOUBLE,          1, 1)
>+JS_DEFINE_CALLINFO_2(DOUBLE, Math_pow,    DOUBLE, DOUBLE,  1, 1)
>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_sqrt,   DOUBLE,          1, 1)
>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_floor,  DOUBLE,          1, 1)
>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_ceil,   DOUBLE,          1, 1)
>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_log,    DOUBLE,          1, 1)
>+JS_DEFINE_CALLINFO_2(DOUBLE, Math_max,    DOUBLE, DOUBLE,  1, 1)
>+JS_DEFINE_CALLINFO_1(DOUBLE, Math_random, RUNTIME,         0, 0)
>+
>+using namespace nanojit;
>+
>+static const JSTraceableNative math_sin_trcinfo =
>+    { js_math_sin,    &ci_Math_sin,    "",    "d",    INFALLIBLE };
>+static const JSTraceableNative math_cos_trcinfo =
>+    { js_math_cos,    &ci_Math_cos,    "",    "d",    INFALLIBLE };
>+static const JSTraceableNative math_pow_trcinfo =
>+    { js_math_pow,    &ci_Math_pow,    "",    "dd",   INFALLIBLE };
>+static const JSTraceableNative math_sqrt_trcinfo =
>+    { js_math_sqrt,   &ci_Math_sqrt,   "",    "d",    INFALLIBLE };
>+static const JSTraceableNative math_floor_trcinfo =
>+    { js_math_floor,  &ci_Math_floor,  "",    "d",    INFALLIBLE };
>+static const JSTraceableNative math_ceil_trcinfo =
>+    { js_math_ceil,   &ci_Math_ceil,   "",    "d",    INFALLIBLE };
>+static const JSTraceableNative math_random_trcinfo =
>+    { js_math_random, &ci_Math_random, "R",   "",     INFALLIBLE };
>+static const JSTraceableNative math_log_trcinfo =
>+    { js_math_log,    &ci_Math_log,    "",    "d",    INFALLIBLE };
>+static const JSTraceableNative math_max_trcinfo =
>+    { js_math_max,    &ci_Math_max,    "",    "dd",   INFALLIBLE };
>+

Sayrer may have a point here (deleted lines removed):

> static JSFunctionSpec math_static_methods[] = {
> #if JS_HAS_TOSOURCE
>+    JS_FN(js_toSource_str,  math_toSource,        0, 0),
> #endif
>+    JS_FN("abs",            math_abs,             1, 0),
>+    JS_FN("acos",           math_acos,            1, 0),
>+    JS_FN("asin",           math_asin,            1, 0),
>+    JS_FN("atan",           math_atan,            1, 0),
>+    JS_FN("atan2",          math_atan2,           2, 0),
>+    JS_TFN("ceil",          &math_ceil_trcinfo,   1, 0),
>+    JS_TFN("cos",           &math_cos_trcinfo,    1, 0),
>+    JS_FN("exp",            math_exp,             1, 0),
>+    JS_TFN("floor",         &math_floor_trcinfo,  1, 0),
>+    JS_TFN("log",           &math_log_trcinfo,    1, 0),
>+    JS_TFN("max",           &math_max_trcinfo,    2, 0),
>+    JS_FN("min",            math_min,             2, 0),
>+    JS_TFN("pow",           &math_pow_trcinfo,    2, 0),
>+    JS_TFN("random",        &math_random_trcinfo, 0, 0),
>+    JS_FN("round",          math_round,           1, 0),
>+    JS_TFN("sin",           &math_sin_trcinfo,    1, 0),
>+    JS_TFN("sqrt",          &math_sqrt_trcinfo,   1, 0),
>+    JS_FN("tan",            math_tan,             1, 0),
>     JS_FS_END
> };

Should we trace all Math methods? Separate bug for sure, just soliciting opinions.

>-    if (fun->u.n.clasp)
>+    if ((fun->flags & JSFUN_TRACEABLE) && fun->u.n.clasp)
>         ABORT_TRACE("can't trace native constructor");

Missing ! before (fun->flags & JSFUN_TRACEABLE).

>-/*
>- * NB: do not use JS_BEGIN_MACRO/JS_END_MACRO or the do-while(0) loop they hide,
>- * because of the embedded continues below.
>- */

Any reason to delete these (rather than combine or minimize)? They were added after someone did the wrong but otherwise-typical thing they advise against.

Looks good otherwise. One more patch if you want to macroize harder (I would gladly read, probably get tired and rs= at some point -- testing and mechanized editing are more important than review). Either way, r=me with nits picked and dtrace and missing-! bugs fixed.

/be

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to comment #13)
> >+enum JSTNErrType { INFALLIBLE, FAIL_NULL, FAIL_NEG, FAIL_VOID, FAIL_JSVAL };
> >+enum { JSTN_ERRTYPE_MASK = 7, JSTN_MORE = 8 };
> 
> Nit: JSTN_ used by jsscript.h:JSTryNoteKind -- could just share the prefix,
> might consider JS_TF or JS_TN as the JSFunctionSpec "row initializer" macro
> instead of JS_TFN -- or use JSTFN_ as the prefix here, but that would want the
> type name to be JSTraceableFastNative (explicit, but overlong?).

I'll keep JSTraceableNative and share JSTN_ for now.  Changed JS_TFN to JS_TN for consistency.

Can we change the try notes to JSTRY_* in a followup bug?

> Elsewhere (in the .cpp file patches) the marching JS_DEFINE_CALLINFO* soldiers
> are followed by parallel trcinfo defs -- could these be combined profitably
> (sharing actual arguments, reducing repetition and unchecked coupling) with
> more macros?

I think they can, but this patch is already big. I want to stabilize and land it. And the redundancy isn't new in this patch--only more obvious, because the patch moves the redundant bits closer together.  :-\

> >+            fun->u.n.u.clasp)
> >+           ? (char *)fun->u.n.u.clasp->name
> 
> Holdover u.n.u. from before the anonymous union change to jsfun.h -- can you
> test DTRACE on before committing, or get someone at Sun to help?

I reverted to the ugly fun->u.n.u everywhere, then macroized, with assertions as you suggested on IRC. Spinning a dtrace build now.

> Should we trace all Math methods? Separate bug for sure, just soliciting
> opinions.

I think a Math method call in a loop is as good a sign as any that the loop needs to go fast.  The code size cost should be small.

> >-/*
> >- * NB: do not use JS_BEGIN_MACRO/JS_END_MACRO or the do-while(0) loop they hide,
> >- * because of the embedded continues below.
> >- */
> 
> Any reason to delete these (rather than combine or minimize)? They were added
> after someone did the wrong but otherwise-typical thing they advise against.

Ooops -- a previous version of the patch changed those |continue|s to |goto next|s and removed the warnings.  Forgot to restore them when I changed it back.

Comment 15

[Jason Orendorff [:jorendorff]]

I pushed to the Try Server and now I have two new problems.

1.  For some reason the Try Server is not turning on ENABLE_JIT in js/src/Makefile.in.  I'll file a bug against IT on this in the morning.

2.  Without ENABLE_JIT this patch is a train wreck.  Still pondering what to do about this.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #14)
> I'll keep JSTraceableNative and share JSTN_ for now.  Changed JS_TFN to JS_TN
> for consistency.

Sounds good.

> Can we change the try notes to JSTRY_* in a followup bug?

Sure, better prefix for the set of names {CATCH,FINALLY,ITER} indeed.

> I think they can, but this patch is already big. I want to stabilize and land
> it. And the redundancy isn't new in this patch--only more obvious, because the
> patch moves the redundant bits closer together.  :-\

Ok.

Heaps of #ifdef JS_TRACER all around (with macros defined both ways for reduced #ifdef'ing) help the !ENABLE_JIT build?

/be

Comment 17

[Brendan Eich [:brendan]]

Or we ditch ENABLE_JIT configurability -- it's on to stay.

/be

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We need ENABLE_JIT until we have 100% platform coverage on nanojit.  Given the breadth of people using SM on strange platforms, I doubt it'll happen for 1.9.1.  (Right now, we would go red on Mac, due to PPC.)

Comment 19

[Brendan Eich [:brendan]]

Right, portability :-).

PPC under way, so we'll get it eventually. Not sure of timing, but it looks like < 5 months.

#ifdefs and two-way macros, then.

/be

Comment 20

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v6-v7

- address brendan's comments
- should compile without ENABLE_JIT now (still testing on Try Server)
- fix a cast for MSVC
- fix a few bugs (e.g. in v6, generic natives were broken)

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: v7

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 342293 [details] [diff] [review]
v7

Looks good, followup bug could handle the JSTRY_ and JSTN_MORE-hiding macro work all in one if you prefer -- I will gladly r+.

/be

Comment 23

[Jason Orendorff [:jorendorff]]

Pushed http://hg.mozilla.org/tracemonkey/rev/408373135eb3

Filed bug 459141 - Rename JSTN_{CATCH,FINALLY,ITER} to JSTRY_*.

In case anyone missed it, comment 15 issue #1 was not a bug at all-- the Mac Try Server builds both x86 (ENABLE_JIT) and PPC (no ENABLE_JIT).

Comment 24

[Andreas Gal :gal]

No measurable perf impact.

Comment 25

[Brendan Eich [:brendan]]

Followup bug on more macrology to consolidate parallel arrays? Only if it clearly wins.

/be

Comment 26

[Graydon Hoare :graydon]

This patch reintroduced bug 458431, and should have been caught by testNativeMax (it fails on my machine). I've folded the (re-)correction into the pending redux merge, bug 457786, and will resubmit shortly.