423874 - Allocating functions together with JSObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #418443 +++

Currently a native function requires to allocate 2 GC-things, JSFunction holding a pointer to the function, and JSObject that wraps the function. It would be nice to allocate these 2 together as one fat JSObject for better cache locality and less allocations.

Comment 1

[Igor Bukanov]

Attachment: v1

The main idea behind the patch is to split JSFunction into JSNativeFunction and JSInterpretedFunction. The former is allocated together with JSObject while the latter is a proper GC thing holding JSScript.

The rest of changes is a consequences of that.

Comment 2

[Igor Bukanov]

Attachment: v2

The new version passes js shell tests and survives some browser smoke testing. Now I need to figure out how to run mochi tests with the new setup.

Comment 3

[Brendan Eich [:brendan]]

This should block since it blocks a 1.9 blocker. Be good to get b5 testing, not take more chances in an RC.

/be

Comment 4

[Mike Schroepfer]

Setting blocking p1 wrt to comment 3

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 310615 [details] [diff] [review]
v2

> struct JSFunction {
>+    uint16          flags;      /* bound method and other flags, see jsapi.h */
>+};
>+
>+struct JSNativeFunction {
>+    JSObject        object;
>+    JSFunction      function;
>     uint16          nargs;      /* maximum number of specified arguments,
>                                    reflected as f.length/f.arity */
>+    uint16          extra;      /* number of arg slots for local GC roots */
>+    uint16          minargs;    /* minimum number of specified arguments, used
>                                    only when calling fast native */

The JSFunction struct may not pack correctly on popular compilers -- please correct me if I'm wrong. Is that tiny JSFunction struct worth having, or could you just put flags in the two subtypes.

>+    JSNative        native;     /* native method pointer or null */

This use of "native" makes me not want to call a JSNativeFunction *native by that name too -- could use nfun for the latter (e.g. in the generic dispatcher code in jsapi.c).

>+struct JSInterpretedFunction {

I know we've used interpreted, but it's too long and vowel-y -- how about Scripted or Hosted instead? The latter nicely matches six-letter "Native".

>+    JSObject        *object;    /* back-pointer to GC'ed object header */
>+    JSFunction      function;
>+    uint16          nargs;      /* number of arguments */
>+    uint16          nvars;      /* number of local variables */
>+    uint16          spare;      /* reserved for future use */
>+    JSScript        *script;    /* interpreted bytecode descriptor or null */
>+    JSLocalNames    names;      /* argument and variable names */
>     JSAtom          *atom;      /* name for diagnostics and decompiling */
> };
> 
> #define JSFUN_EXPR_CLOSURE   0x4000 /* expression closure: function(x)x*x */
> #define JSFUN_INTERPRETED    0x8000 /* use u.i if set, u.n if unset */
> 
> #define JSFUN_SCRIPT_OR_FAST_NATIVE (JSFUN_INTERPRETED | JSFUN_FAST_NATIVE)
> 
> #define FUN_INTERPRETED(fun) ((fun)->flags & JSFUN_INTERPRETED)

So yeah, INTERPRETED here and nearby would want to track the name change. Scripted (sfun) and Hosted (hfun, but that's confusing if we have "Host functions" aka native functions defined by the embedding host, too) suggest FUN_SCRIPTED, etc.

Rest looked good at a fast skim but I'll read more closely with the above class of nits picked. Thanks,

/be

Comment 6

[Igor Bukanov]

Attachment: v3

The new version addresses the nits and uses "scripted", not "interpreted", to name macros/functions.

With the patch JSFunction is no longer defined and is used as a typed pointer to the "flags" field of JS(Native|Scripted)Function.

Comment 7

[Igor Bukanov]

Attachment: v4

The new version uses public API, not internal macros, to access JSFunction fields outside of SpiderMonkey.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 310762 [details] [diff] [review]
v4

>-#include "jsfun.h"
> #include "jsobj.h"
>+#include "jsfun.h"

This should not be necessary -- nest "jsobj.h" in "jsfun.h" instead. Headers should hide their implementation depenedencies. Secondary good is alphabetization of the "main sequence" #include "js*.h" directives. Same goes for other files.

>+++ js/jsd/jsd_xpc.cpp	20 Mar 2008 15:51:02 -0000
>@@ -1028,26 +1028,28 @@ jsdScript::CreatePPLineMap()
>     JSAutoRequest ar(cx);
>     JSObject   *obj = JS_NewObject(cx, NULL, NULL, NULL);
>     JSFunction *fun = JSD_GetJSFunction (mCx, mScript);
>     JSScript   *script;
>     PRUint32    baseLine;
>     PRBool      scriptOwner = PR_FALSE;
>     
>     if (fun) {
>-        if (fun->nargs > 12)
>+        PRUint32 nargs = JS_GetFunctionArity(fun);

Does this relieve something in js/jsd from #include'ing "jsfun.h"?

>Index: js/src/js.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/js.c,v
>retrieving revision 3.208
>diff -U8 -p -r3.208 js.c
>--- js/src/js.c	17 Mar 2008 20:25:23 -0000	3.208
>+++ js/src/js.c	20 Mar 2008 15:51:02 -0000

Likewise, if js.c can do without including "jsfun.h" now, great. Or was this change to use now-mandatory JS_* API calls? It seemed more than that.

>+JSScriptedFunction *
>+js_NewScriptedFunction(JSContext *cx, JSObject *funobj, uintN flags,
>+                       JSObject *parent, JSAtom *atom)
> {
>-    JSFunction *fun;
>+    JSScriptedFunction *fun;
...
>+JSNativeFunction *
>+js_NewNativeFunction(JSContext *cx, JSNative native, uintN nargs,
>+                     uintN flags, JSObject *parent, JSAtom *atom)
>+{
>+    JSNativeFunction *fun;

These two don't diff usefully, so may as well use sfun and nfun, respectively -- I see why elsewhere to cut down on diff size and noise-line you avoided renaming from fun, but canonical variable names win in SpiderMonkey, helping grep users like myself do quick checks, and adding a bit more consistency to help readers.

>+    if (FUN_IS_SCRIPTED(fun)) {
>+        JSScriptedFunction *ifun;
>+
>+        ifun = FUN_TO_SCRIPTED(fun);

sfun, in light of the above -- and check for other ifun uses (I see more below).

>+#define FUN_FLAGS(fun)       (*(uint16 *) (fun))
>+
>+#define FUN_IS_SCRIPTED(fun) (FUN_FLAGS(fun) & JSFUN_SCRIPTED)
>+
>+#define FUN_TO_SCRIPTED(fun)                                                  \
>+    (JS_ASSERT(FUN_IS_SCRIPTED(fun)),                                         \
>+     (JSScriptedFunction *)((uint8 *)fun - offsetof(JSScriptedFunction, flags)))
> 
>-#define JSFUN_SCRIPT_OR_FAST_NATIVE (JSFUN_INTERPRETED | JSFUN_FAST_NATIVE)
>+#define FUN_TO_NATIVE(fun)                                                    \
>+    (JS_ASSERT(!FUN_IS_SCRIPTED(fun)),                                        \
>+     (JSNativeFunction *)((uint8 *)fun - offsetof(JSNativeFunction, flags)))

A block comment about the magic JSFunction opaque pointer to flags common member of JS{Scripted,Native}Function would be good here, even if done elsewhere. Here's where the magic starts.

>+++ js/src/jsprvtd.h	20 Mar 2008 15:51:12 -0000
>@@ -87,21 +87,23 @@ typedef uint8  jsbytecode;
> typedef uint8  jssrcnote;
> typedef uint32 jsatomid;
> 
> /* Struct typedefs. */
> typedef struct JSArgumentFormatMap  JSArgumentFormatMap;
> typedef struct JSCodeGenerator      JSCodeGenerator;
> typedef struct JSGCThing            JSGCThing;
> typedef struct JSGenerator          JSGenerator;
>+typedef struct JSNativeFunction     JSNativeFunction;
> typedef struct JSParseContext       JSParseContext;
> typedef struct JSParsedObjectBox    JSParsedObjectBox;
> typedef struct JSParseNode          JSParseNode;
> typedef struct JSPropCacheEntry     JSPropCacheEntry;
> typedef struct JSSharpObjectMap     JSSharpObjectMap;
>+typedef struct JSScriptedFunction   JSScriptedFunction;

Sort before JSSharpObjectMap.

>@@ -1448,25 +1448,25 @@ js_NewScriptFromCG(JSContext *cx, JSCode
>     if (cg->ntrynotes != 0)
>         js_FinishTakingTryNotes(cg, JS_SCRIPT_TRYNOTES(script));
>     if (cg->objectList.length != 0)
>         FinishParsedObjects(&cg->objectList, JS_SCRIPT_OBJECTS(script));
>     if (cg->regexpList.length != 0)
>         FinishParsedObjects(&cg->regexpList, JS_SCRIPT_REGEXPS(script));
> 
>     /*
>-     * We initialize fun->u.script to be the script constructed above
>-     * so that the debugger has a valid FUN_SCRIPT(fun).
>+     * We initialize fun->script to be the script constructed above
>+     * so that the debugger has a valid fun->script.

This is a bit redundant and lengthy, suggest "Initialize fun->script early for the debugger."

r=me with these picked and of course tests all good.

/be

Comment 9

[Brendan Eich [:brendan]]

And thanks for taking the bull by the horns -- there was no quick and dirty hack, this is the right thing.

/be

Comment 10

[Igor Bukanov]

Attachment: v5

The new version addresses the nits from the comment 8.

Comment 11

[Damon Sicore (:damons)]

Comment on attachment 310878 [details] [diff] [review]
v5

a1.9b5+=damons

Comment 12

[Igor Bukanov]

I checked in the patch from the comment 10 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206087482&maxdate=1206087601&who=igor%25mir2.org

Comment 13

[Igor Bukanov]

Given the compatibility problems discovered in bug 424376 the changes introduced in this bug are wrong. The observation is that for compatibility JSFunction should always have the corresponding object. Thus what I should have done instead was to always allocate JSFunction together with JSObject both for native and scripted case. Then for clones the standard JSObject would be allocated with the private slot pointing to the original fat object.

This would made the patch smaller, avoid compatibility problems and eliminate JSTRACE_FUNCTION while reducing the number of allocated and traced GC things.

Thus I suggest to backout the landed patch and create smaller patch that would follow the above idea.

Comment 14

[Igor Bukanov]

I backed out the patch from the comment 10 per comment 13.

Comment 15

[Igor Bukanov]

Attachment: v6

The patch uses fat objects to allocates the function fields after the object. For cloned function objects the code allocates the normal JSObject. This is transparent since in both cases the private slot of either fat or normal object points to JSFunction.

With this change the function becomes JSTRACE_OBJECT from the GC point of view eliminating the need for JSTRACE_FUNCTION.

In the patch I have added FUN_OBJECT macro so in C++ the code can extend JSFunction from JSObject and define FUN_OBJECT(fun) accordingly.

Comment 16

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

I don't understand how this will prevent the function-collected-before-script problem that we had in 424376, unless we're sure that we will only inform the debugger about the "primary" function.  If the debugger ever gets notified about a script and a cloned function, we're right back where we were.

(Maybe we're thusly sure, I haven't walked though the patch in enough detail to spot it.  Maybe an assertion that we only call the debugger hook with primary functions?)

Comment 17

[Brendan Eich [:brendan]]

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

gc thing dumping casts JS_GetPrivate result to (JSFunction*) -- use the macro?

fun_resolve overlong line (80th column intrusion alert!):
+        proto = js_NewObject(cx, &js_ObjectClass, NULL, OBJ_GET_PARENT(cx, obj),

fun_xdrObject if ((n = ...) != 0) nit -- avoid nesting assignment in if
condition, try to nest only in while/do-while and somtimes for conditions.

Nit: fun_finalize leaves out u. before i.script in comment:
     * Null-check of i.script is required since the parser sets interpreted

Confusion: in js_NewFunction we have

        funobj = js_NewObject(cx, &js_FunctionClass, NULL, parent, 0);

but in js_CloneFunctionObject we have

    newfunobj = js_NewObject(cx, &js_FunctionClass, NULL, parent,
                             sizeof(JSObject));

so (indeed it is) js_NewObjectWithGivenProto must be doing some class-specific
special-casing. It would be better to put that random logic in callers, or else
make it universally class-based (no if/else tests). The new-object code path
performance is sensitive, but perhaps this test does not perturb benchmark
timings.

extra vs. size issue is secondary (an ADD op won't matter) but I wonder about
the conditional memset to zero, instead of a free pattern in DEBUG builds and
obligating callers to initialize extra.

/be

Comment 18

[Igor Bukanov]

(In reply to comment #16)
> I don't understand how this will prevent the function-collected-before-script
> problem that we had in 424376, unless we're sure that we will only inform the
> debugger about the "primary" function.

The bug 424376 happens because initial versions of the function changes violated the implicit promise of API that a function is alive as long as the script obrained via JS_GetFunctionScript(c, fun) is alive. This happens because the cloned functions also became JSFunction instances for the user of public API.

The new patch does not violate the promise since JSFunction is always the original non-cloned function. The clones are ordinary JSObject instances that as before points to the original JSFunction via the private slot.

Effectively the patch is optimization based on the observation that js_NewFunction always allocates JSFunction and its JSObject wrapper together. Moreover, this original non-cloned wrapper is finalized if and only if the corresponding JSFunction is finalized. As such it is possible to allocate the two together removing the extra level of indirection. 

Comment 19

[Igor Bukanov]

Attachment: v7

The new version initializes the space beyound JSObject in the caller (js_NewFunction), not in js_NewObjectWithGivenProto.

The patch still checks for js_FunctionClass in js_NewObjectWithGivenProto as an alternative would be to duplicate the checks in JS_InitClass, js_InvokeConstructor and JS_NewSystemObject.

The new version also eliminates GET_FUNCTION_PRIVATE for functions stored in JSScript. They are not cloned so the simple cast to (JSFunction *) is enough.

Comment 20

[Brendan Eich [:brendan]]

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

80th column violations, at a glance:

+    return js_CloneFunctionObject(cx, GET_FUNCTION_PRIVATE(cx, funobj), parent);
+    /* Set private to self to indicate non-cloned fully initialized function. */

Looks good otherwise. We can generalize the object allocation code (and optimize it more) later. Thanks,

/be

Comment 21

[Igor Bukanov]

(In reply to comment #20)
> (From update of attachment 312604 [details] [diff] [review])
> 80th column violations, at a glance:
> 
> +    return js_CloneFunctionObject(cx, GET_FUNCTION_PRIVATE(cx, funobj),
> parent);
> +    /* Set private to self to indicate non-cloned fully initialized function.
> */

In both of these cases the length of lines is exactly 80 chars.

Comment 22

[Brendan Eich [:brendan]]

No big deal -- we try to avoid putting anything in column 80 (tw=78 in vim's modeline) but if it doesn't break nicely, nm.

/be

Comment 23

[Igor Bukanov]

(In reply to comment #22)
> No big deal -- we try to avoid putting anything in column 80 (tw=78 in vim's
> modeline) but if it doesn't break nicely, nm.

Hm, there are quite a few lines with 80 chars that my changes in past produced since I thought the limit is 80, no 79.

Comment 24

[Brendan Eich [:brendan]]

The limit was 79 historically for Emacs, which puts a line-wrap indicator even if there is a newline right after the non-newline 80th char on the line (numbering columns from one, this is why the \ for macros is in column 79 not 80).

It's not a big deal, of course. Main usability concern is avoiding overlong or varying line limits, which hurt readability. We could go higher, but let's do it at once. All else equal, 79 seems best for now.

/be

Comment 25

[Igor Bukanov]

I checked in the patch from the comment 19 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1207122360&maxdate=1207122361&who=igor%25mir2.org

Comment 26

[Jason Orendorff [:jorendorff]]

Brendan, do you mean tw=79 ?  I tried tw=78; it avoids putting anything in column 79 (counting from 1).  That is, it puts at most 78 characters on a line, not counting the '\n'.

I have '(fill-column 78) in my .emacs file.  Same effect.

Another possible source of confusion:  Emacs column-number-mode counts columns from 0, not 1.

Comment 27

[Brendan Eich [:brendan]]

I must mean tw=79 -- I've been relying on mrbkap's vim-fu being better than my old-school northern-star style. Blake, should we fix all the tw=78 vim modelines to use 79?

/be