+++ 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.

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.

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.

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

Setting blocking p1 wrt to comment 3

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

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.

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

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

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

The new version addresses the nits from the comment 8.

Comment on attachment 310878 [details] [diff] [review] v5 a1.9b5+=damons

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

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.

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

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.

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 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

(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.

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 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

(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.

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

(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.

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

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

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.

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