322001 - JSFunction without nref

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Since JSFunction is allocated from GC heap, there is no need to keep nref reference counter in JSFunction. fun_finalize can use js_IsAboutToBeFinalized to check when script from JSFunction instance should be destroyed.

AFAICS the only drawback is that JS_GetFunctionTotalSize would not be able to account for all cloned function objects.

Comment 1

[Igor Bukanov]

Attachment: Implementation

Comment 2

[Brendan Eich [:brendan]]

Yeah, I left nrefs there for JS_GetFunctionTotalSize, so each function object takes a 1/nrefs share in the size of its JSFunction.  That's used by XPConnect's profiler:

http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/tools/src/nsXPCToolsProfiler.cpp#219

Is this used anywhere (Venkman)?

Thinking only about API compatibility, it's worth keeping nrefs around.  And it does not cost extra space, since on 32-bit architectures, JSFunction is 8 words with nrefs, 7 without, and the GC allocates in two-word increments.

On the plus side, the patch eliminates refcounting costs at the cost of a gc-thing flag address computation and fetch, and consolidates mechanism.

Comments from peanut gallery welcome.

/be

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)

> On the plus side, the patch eliminates refcounting costs at the cost of a
> gc-thing flag address computation and fetch,

The former cost when creating and cloning functions as well as at finalization; the latter only on each function object finalization.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #2)
> And it
> does not cost extra space, since on 32-bit architectures, JSFunction is 8 words
> with nrefs, 7 without, and the GC allocates in two-word increments.

AFAICS JSFunction.nregexps makes sence only for interpreted scripts where JSFunction.extra is not used. This allows to save uint16. And if one can take away   spare, then JSFunction would be reduced by 2 words with memory shrink for real.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> AFAICS JSFunction.nregexps makes sence only for interpreted scripts where
> JSFunction.extra is not used. This allows to save uint16. And if one can take
> away   spare, then JSFunction would be reduced by 2 words with memory shrink
> for real.

Good point -- how about a new patch that does s/nregexps/extra/g and gets rid of spare?  Then this patch is a win in space as well as time.

The JS_GetFunctionTotalSize API could have its API compatibility restored if we could count all function objejcts sharing a JSFunction.  Since it appears that nothing uses the JS_GetFunctionTotalSize API in normal operation, putting the burden on its implementation is fair, so long as it can be done efficiently.

Without prior arrangement, the only way to reimplement JS_GetFunctionTotalSize and compute 1/nrefs is to do a global GC-like procedure to find all function objects sharing the JSFunction.  That's inefficient to do on each call, so we'd want to build up a secret size database to answer requests for other JSFunctions' total sizes.  Then we'd have to worry about invalidating this size db, or updating its entries on clone and finalize operations.

With prior arrangement, the jsdbgapi.c could keep its size database for all functions.  The existing debugger hooks are not sufficient, since no hook is called from js_CloneFunctionObject.  Still, if the profiler uses optional hooks, this sounds like the better way.  Comments welcome.

/be

Comment 6

[Igor Bukanov]

Attachment: More compact JSFunction

Since jsxml.c uses JSFunction.spare, it is not possible to remove this member. Still JSFunction can be reduced as only the interpreted case uses JSFunction.nvars and JSFunction.nregexps while only the native methods needs extra and spare fields. 

Thus the patch besides removal of nref changes JSFunction to:

struct JSFunction {
    JSObject     *object;       /* back-pointer to GC'ed object header */
    uint16       nargs;         /* minimum number of actual arguments */
    uint8        flags;         /* bound method and other flags, see jsapi.h */
    JSPackedBool interpreted;   /* use u.script if true, u.native if false */
    union {
        struct {
            uint16   extra;     /* number of arg slots for local GC roots */
            uint16   spare;     /* reserved for future use */
            JSNative native;    /* native method pointer or null */
        } n;
        struct {
            uint16   nvars;     /* number of local variables */
            uint16   nregexps;  /* number of regular expressions literals */
            JSScript *script;   /* interpreted bytecode descriptor or null */
        } i;
    } u;
    JSAtom       *atom;         /* name for diagnostics and decompiling */
    JSClass      *clasp;        /* if non-null, constructor for this class */
};

and adjust the rest of code accordingly to use either JSFunction.u.n for native-only members and JSFunction.u.i for the interpreted case.

Comment 7

[Igor Bukanov]

(In reply to comment #5)

If keeping proper JS_GetFunctionTotalSize turned out to be too difficult, then at least nref can be moved to JSScript since only non-native functions can be cloned. In this way JSFunction would be shrunk by 2 words while increasing JSScript by one. 

Even that can be avoided if "jsbytecode   *code;" in JSScript would be moved to the end of JSScript and declared as "jsbytecode   code[1]" to emphasis that that bytecode always comes after the script struct.

Comment 8

[Igor Bukanov]

Attachment: More compact JSFunction v2

The previous version of the patch would write garbage in place of extra field in fun_xdrObject. This version writes 0 and checks that the read value is also zero as it should be for all seriolized interpreted functions.

But I should remind myself that I should stop finding timeslots for SpiderMonkey between meeting New Year (here it is already 2006 for more then 2 hours ;) and preprations for vacation trip to Tenerife (Canarian islands).

Hapy New Year selebrations!

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> If keeping proper JS_GetFunctionTotalSize turned out to be too difficult, then
> at least nref can be moved to JSScript since only non-native functions can be
> cloned. In this way JSFunction would be shrunk by 2 words while increasing
> JSScript by one. 

I took another look at JS_GetFunctionTotalSize and it's all messed up, due to old parameterization mistake that goes back to before there was a JSFunction.nrefs or function object cloning (I mean that the mistake goes back to that primordial era, not JS_GetFunctionTotalSize -- it came in early 2001, and should have been done better).

JS_GetFunctionTotalSize takes a JSFunction *fun parameter, so it can only add the the total size of fun->object, not of the particular object (clone- or proto- function, typically fun->object is proto-function) that is of interest to the profiler.  Worse, it divides the total size of fun->object by nrefs.  It seems to be assuming that there is one fun->object for many (nrefs) funs, but that's backward.  Also, each function object sharing a fun is not the same total size: the clones are smaller by a lot (they share the proto-scope and have no ad-hoc or hidden [arg, var] properties).

So it's really just buggy, and I think this bug should get rid of obytes and just add JS_GetObjectTotalSize(cx, obj) if obj is non-null.

> Even that can be avoided if "jsbytecode   *code;" in JSScript would be moved to
> the end of JSScript and declared as "jsbytecode   code[1]" to emphasis that
> that bytecode always comes after the script struct.

This would break JS_ExecuteScriptPart, which needs to fake a script with code pointing at the main entry point of the passed-in script's code.

/be

Comment 10

[Igor Bukanov]

Attachment: More compact JSFunction v3

Here is a patch version to address comment 9 and remove obytes from JS_GetFunctionTotalSize. nsXPCToolsProfiler.cpp calls JS_GetFunctionTotalSize only from JSNewScriptHook where with old code nref == 1 so the change would not even be visible through xpconnect users.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 207927 [details] [diff] [review]
More compact JSFunction v3

>+    if (fun->interpreted && fun->u.i.script) {
>+        if (js_IsAboutToBeFinalized(cx, fun)) {
>+            script = fun->u.i.script;
>+            fun->u.i.script = NULL;
>+            js_DestroyScript(cx, script);
>+        }
>+    }

Nit: the prevailing style almost always avoids if(C1)-if(C2)-then in favor of if(C1&&C2)-then, to save indentation and bracing.

>+    uint16 extraDummy;          /* variable to read no longer used field */

May want to call this something that indicates it could be used in a future rev of the XDR format, such as unused or spare.

>     JSPackedBool interpreted;   /* use u.script if true, u.native if false */

Update comment to say "use u.i if true, u.n if false" or equivalent.

Nice struct packing -- looks good to me on 32- and 64-bit systems.

Excellent patch, rock on!  Please check in ASAP, I have some pending changes that will want to merge with this.  Thanks,

/be

Comment 12

[Igor Bukanov]

Attachment: Patch to commit

This fixes comments and addresses nits from above.

Comment 13

[Igor Bukanov]

I committed the last patch from attachment 207964 [details] [diff] [review]