Improve internal API for traceable natives

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Trunk
mozilla1.9.1b2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 341924 [details] [diff] [review]
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.
(Assignee)

Updated

10 years ago
Blocks: 453738
(Assignee)

Comment 1

10 years ago
Created attachment 341930 [details] [diff] [review]
v1 (against tracemonkey)
Assignee: general → jorendorff
Attachment #341924 - Attachment is obsolete: true
Attachment #341930 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #341930 - Flags: review? → review?(shaver)
(Assignee)

Comment 2

10 years ago
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 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
(Assignee)

Comment 4

10 years ago
Created attachment 341965 [details] [diff] [review]
v2

Addressing brendan's comments.
Attachment #341930 - Attachment is obsolete: true
Attachment #341965 - Flags: review?(shaver)
Attachment #341930 - Flags: review?(shaver)

Updated

10 years ago
Attachment #341965 - Flags: review?(edwsmith)

Comment 5

10 years ago
I like the departure from fid-based builtin identification. Ed, what do you think?
(Assignee)

Comment 6

10 years ago
Created attachment 341991 [details] [diff] [review]
v3

This should break the browser build a bit less.
Attachment #341965 - Attachment is obsolete: true
Attachment #341991 - Flags: review?(shaver)
Attachment #341965 - Flags: review?(shaver)
Attachment #341965 - Flags: review?(edwsmith)
Comment on attachment 341991 [details] [diff] [review]
v3

Brendan better to review this in a timely fashion, and Ed for the nanojit changes.
Attachment #341991 - Flags: review?(shaver)
Attachment #341991 - Flags: review?(edwsmith)
Attachment #341991 - Flags: review?(brendan)
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

10 years ago
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).
Attachment #341991 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 10

10 years ago
Created attachment 342081 [details] [diff] [review]
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
Attachment #341991 - Attachment is obsolete: true
Attachment #342081 - Flags: review?(brendan)
Attachment #341991 - Flags: review?(brendan)
(Assignee)

Comment 11

10 years ago
Created attachment 342087 [details] [diff] [review]
v5 - update to tip

Fix conflicts with bz's patch for |new Date|, which landed yesterday.
Attachment #342081 - Attachment is obsolete: true
Attachment #342081 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Attachment #342087 - Flags: review?(brendan)
(Assignee)

Comment 12

10 years ago
Created attachment 342142 [details] [diff] [review]
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.
Attachment #342087 - Attachment is obsolete: true
Attachment #342142 - Flags: review?(brendan)
Attachment #342087 - Flags: review?(brendan)

Updated

10 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9.1b2
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
Attachment #342142 - Flags: review?(brendan) → review+
(Assignee)

Comment 14

10 years ago
(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.
(Assignee)

Comment 15

10 years ago
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.
(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
Or we ditch ENABLE_JIT configurability -- it's on to stay.

/be
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.)
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
(Assignee)

Comment 20

10 years ago
Created attachment 342281 [details] [diff] [review]
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)
(Assignee)

Comment 21

10 years ago
Created attachment 342293 [details] [diff] [review]
v7
Attachment #342142 - Attachment is obsolete: true
Attachment #342293 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Blocks: 459085
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
Attachment #342293 - Flags: review?(brendan) → review+
(Assignee)

Comment 23

10 years ago
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).
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 24

10 years ago
No measurable perf impact.
Followup bug on more macrology to consolidate parallel arrays? Only if it clearly wins.

/be
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.

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.