Closed Bug 399544 Opened 12 years ago Closed 12 years ago

Custom storage for function argument and variable names

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

Attachments

(1 file, 10 obsolete files)

Currently to store the names of function arguments and variables the code uses hidden properties stored in JSObject referenced bu JSFunction.object. That forces creation of a scope object for each interpreted function. That requires to allocate at least 10/9 words on 32/64 bit platforms even if the function has just few parameters and variables. In addition the space is taken by JSScopeProperty instances. Although they are shared, due to the size of JSScopeProperty it adds more overhead.

It would be nice to replace the hidden properties with a specialized storage to reduce the overhead.
Attached patch v0.5 (obsolete) — Splinter Review
Here is a backup of something that compiles. The patch depends on the patch from bug 398609 comment 11.
Attached patch v1 (quilt patch) (obsolete) — Splinter Review
[The patch should be applied on the top of the patch from bug 398609 comment 11.]

The patch replaces the hidden properties added to JSFunction.object via a custom structure in JSFunction. To make room for the structure the patch moves JSFunction.clasp into native-only part of JSFunction and uses JSLocalNames union to store the names of arguments and variable in the interpreted part of JSFunction.

Depending on the value of n == JSFunction.nargs + JSFunction.u.i.nvars the name is stored either directly (n == 1) or in a small array (n <= MAX_ARRAY_LOCALS) or in a hashtable. In the first 2 cases the name is stored as a tagged atom with the lowest bit set when the name corresponds to const declaration. In the third case, when a hashtable is used, a separated singly-linked list stores the duplicated parameter names.

The patch also adds js_FreezeLocalNames function to shrink the structure holding local names after all the names are parsed or xdr-decoded.

The patch passes the shell test suite/bloatcycle tests and despite the extra code the size of an optimized js shell executable on Linux is still lower then the size before the patch for 398609 is applied.
Attachment #284572 - Attachment is obsolete: true
Attached patch v1b (quilt patch) (obsolete) — Splinter Review
This is a CVS sync. The patch requires the patch from bug 398609 comment 12.
Attachment #285241 - Attachment is obsolete: true
Attached patch v1c (quilt patch) (obsolete) — Splinter Review
The patch update to sync with changes from bug 398609 comment 22.
Attachment #285539 - Attachment is obsolete: true
Attached patch v1d (quilt patch) (obsolete) — Splinter Review
A new version adds/fixes comments to better reflect the implementation. It should be applied on top of the patch from bug 398609 comment 22.
Attachment #286471 - Attachment is obsolete: true
Attached patch v1e (obsolete) — Splinter Review
This a CVS diff for a review. For details see comment 2.
Attachment #286483 - Attachment is obsolete: true
Attachment #288496 - Flags: review?(brendan)
Blocks: 398219
the patch decreases by 0.7% the start up time on a 1.1GHz Pentium-M laptop from 1.43 to 1.42 seconds. 
New patch needed since 11-14? I will review ASAP based on advice, thanks.

/be
Attached patch v2 (obsolete) — Splinter Review
Attachment #289341 - Flags: review?
Comment on attachment 289341 [details] [diff] [review]
v2

The patch update to sync with CVS.
Attachment #289341 - Flags: review? → review?(brendan)
Attachment #288496 - Attachment is obsolete: true
Attachment #288496 - Flags: review?(brendan)
Comment on attachment 289341 [details] [diff] [review]
v2

Looks good, although I read quickly (short on time due to travels and vacation). A couple of things:

Nit: else after return in js_LookupLocal for the |if (i < fun->nargs)| test.

Shouldn't js_FreezeLocalNames test n <= MAX_ARRAY_LOCALS, not <, here?

    if (2 <= n && n < MAX_ARRAY_LOCALS) {

/be
Attached patch v3 (obsolete) — Splinter Review
The new version addresses the nit from comment 11.
Attachment #289341 - Attachment is obsolete: true
Attachment #290154 - Flags: review?(brendan)
Attachment #289341 - Flags: review?(brendan)
(In reply to comment #11)
> (From update of attachment 289341 [details] [diff] [review])
> Shouldn't js_FreezeLocalNames test n <= MAX_ARRAY_LOCALS, not <, here?
> 
>     if (2 <= n && n < MAX_ARRAY_LOCALS) {

js_FreezeLocalNames needs to shrink the array not to waste the tail space only when n < MAX_ARRAY_LOCALS. When n == MAX_ARRAY_LOCALS all the allocated space is used.
Comment on attachment 290154 [details] [diff] [review]
v3


> static JSBool
> call_enumerate(JSContext *cx, JSObject *obj)
> {
[snip]
>+        /* Trigger reflection by looking up the name. */
>+        if (!js_LookupProperty(cx, obj, ATOM_TO_JSID(name), &pobj, &prop)) {
>+            names = NULL;
>+            goto out;
>+        }
>+        JS_ASSERT(prop);
> 
>         /*
>          * If we found the property in a different object, don't try sticking
>          * it into wrong slots vector. This can occur because we have a mutable
>          * __proto__ slot, and cloned function objects rely on their __proto__
>          * to delegate to the object that contains the var and arg properties.
>          */
>-        if (!prop || pobj != obj) {
>-            if (prop)
>-                OBJ_DROP_PROPERTY(cx, pobj, prop);
>-            continue;
>+        if (pobj == obj) {

Is it possible for pobj != obj here? The comment above seems out of date, since we do not depend on function object (scope) properties for local names any longer, in any event. But since Call objects have a null __proto__ I think we also can't have a non-error return from js_LookupProperty that does not find name in the direct Call object's scope. Right?

Sorry for the brain fart on js_FreezeLocalNames, duh! Rest of patch looks great, thanks for writing it.

/be
(In reply to comment #14)
> >         /*
> >          * If we found the property in a different object, don't try sticking
> >          * it into wrong slots vector. This can occur because we have a mutable
> >          * __proto__ slot, and cloned function objects rely on their __proto__
> >          * to delegate to the object that contains the var and arg properties.
> >          */
> >-        if (!prop || pobj != obj) {
...
> >+        if (pobj == obj) {
> 
> Is it possible for pobj != obj here?

You are right and in fact that change should be done as part of bug 398609 as that patch made very clear what is going on in call_enumerate.
Attached patch v4 (obsolete) — Splinter Review
this is v3 + cleanup in call_enumerate.
Attachment #290154 - Attachment is obsolete: true
Attachment #290204 - Flags: review?(brendan)
Attachment #290154 - Flags: review?(brendan)
Comment on attachment 290204 [details] [diff] [review]
v4

>diff -U8 -p -r3.233 jsfun.c
>@@ -823,20 +820,16 @@ call_resolve(JSContext *cx, JSObject *ob
>     fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
>     if (!fp)
>         return JS_TRUE;
>     JS_ASSERT(fp->fun);
> 
>     if (!JSVAL_IS_STRING(id))
>         return JS_TRUE;
> 
>-    if (!fp->callee)
>-        return JS_TRUE;
>-    JS_ASSERT(GET_FUNCTION_PRIVATE(cx, fp->callee) == fp->fun);
>-

I removed this code as fp->callee is not used in the code. But may be it is better to remove just the check while keeping the assert while adding the same assert to call_enumerate for symmetry.
Attached patch v5 (obsolete) — Splinter Review
This is the previous patch with an extra asserts in call_enumerate and call_resolve.
Attachment #290204 - Attachment is obsolete: true
Attachment #290212 - Flags: review?(brendan)
Attachment #290204 - Flags: review?(brendan)
Comment on attachment 290212 [details] [diff] [review]
v5

>     fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
>     if (!fp)
>         return JS_TRUE;
> 
>-    /*
>-     * Do not enumerate a cloned function object at fp->callee, it may have
>-     * gained its own (mutable) scope (e.g., a brutally-shared XUL script sets
>-     * the clone's prototype property).  We must enumerate the function object
>-     * that was decorated with parameter and local variable properties by the
>-     * compiler when the compiler created fp->fun, namely fp->fun->object.
>-     *
>-     * Contrast with call_resolve, where we prefer fp->callee, because we'll
>-     * use js_LookupProperty to find any overridden properties in that object,
>-     * if it was a mutated clone; and if not, we will search its prototype,
>-     * fp->fun->object, to find compiler-created params and locals.
>-     */
>-    funobj = fp->fun->object;
>-    if (!funobj)
>-        return JS_TRUE;
>+    JS_ASSERT(GET_FUNCTION_PRIVATE(cx, fp->callee) == fp->fun);

Could crunch the newline out of the above to match call_resolve style, making a single paragraph of

    fp = (JSStackFrame *) JS_GetPrivate(cx, obj);
    if (!fp)
        return JS_TRUE;
    JS_ASSERT(GET_FUNCTION_PRIVATE(cx, fp->callee) == fp->fun);

>+        /*
>+         * At this point the call object always has a property corresponding
>+         * to the local name as call_resolve creates the property using
>+         * JSPROP_PERMANENT.

s/as/because/

r+a=me with these nits picked.

/be
Attachment #290212 - Flags: review?(brendan)
Attachment #290212 - Flags: review+
Attachment #290212 - Flags: approval1.9+
Attached patch v5bSplinter Review
Addressing the nits
Attachment #290212 - Attachment is obsolete: true
Attachment #290358 - Flags: review+
Attachment #290358 - Flags: approval1.9+
i checked in the patch from comment 20 to the CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1196152633&maxdate=1196152800&who=igor%mir2.org
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 406079
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.