411575 - js_PutCallObject() is slow.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Johnny Stenback (:jst)]

See bug 411571, in that testcase (which is admittedly not something you'd see in the real world, but this part of it applies to a lot of AJAXy code out there as nested function calls are pretty common in AJAX libraries). To recap, out of all the time we spend in JS in that testcase we spend ~21.5% in js_PutCallObject(). The breakdown of js_PutCallObject() in that profile looks something like this:

% of    # of    Function
scope   calls   called
100.00  200006  js_PutCallObject
65.44   400011  malloc
52.48   200006  js_GetLocalNames
51.57   200006  JS_ArenaAllocate
41.28   600061  js_LookupProperty
41.13   600061  js_LookupPropertyWithFlags
27.91   600040  js_DefineNativeProperty
15.25   600040  js_GetMutableScope
14.82   200005  js_NewScope
14.21   200005  JS_malloc
11.93   600040  js_AddScopeProperty
8.69    600040  js_AllocSlot
8.15    200013  JS_realloc
8.08    200013  realloc
5.97    2000112 @JS_DHashTableOperate
5.62    600028  js_AtomizeString
5.38    200006  JS_ArenaRelease
5.24    200006  free
3.05    600040  js_StopResolving
2.78    800036  PR_Unlock
2.56    1000051 memset
2.39    1600072 MD_CURRENT_THREAD
2.16    1600072 TlsGetValue
2.02    800036  PR_Lock
1.81    600040  JS_DHashTableRawRemove
...

Note that this is optimized code, so some code gets inlined etc. The breakdown of the time spent in the immediate descendants of js_PutCallObject() looks like this:

Function		%	# of calls
js_GetLocalNames	52.48	200006
js_LookupProperty	41.28	600025
JS_ArenaRelease		5.38	200006
JS_GetPrivate		0.07	200006
js_DropProperty		0.06	600025
JS_SetPrivate		0.04	200006
js_SetProperty		0.00	4
js_PutArgsObject	0.00	4

Igor, Brendan says you've been in this code lately and could probably optimize this a fair bit. So hence I'm giving this bug to you.

Comment 1

[Brendan Eich [:brendan]]

I mentioned that SpiderMonkey is totally unoptimized as far as closure capture when the compiler sees a function body reference a non-local name: any outer functions will be marked JSFUN_HEAVYWEIGHT and eagerly gain call objects when they are invoked.

Call objects are themselves optimized so that active heavyweight functions can use the interpreter stack directly when loading and storing arguments and local vars, and any references that go through the call object will reflect on the interpreter stack (call_resolve). But when the function returns, the values about to be freed from the stack must be homed to the call object, and this is done using brute force and ignorance (call_enumerate from js_PutCallObject, etc.).

So two broad avenues of attack:

1. Use standard Scheme compiler techniques to avoid forming full closures, instead binding to outer names' values in their stack frames, if you can prove that the outer names are used only when the outer function is still active (no upward funarg).

2. Optimize the existing runtime code to be faster than it is. This could even involve eliminating Call *objects*, using copied stack frames or other just-so native data structures.

Approach 2 fits in the time remaining better, but may yield fewer speedups. Still lots to do here.

/be

Comment 2

[Igor Bukanov]

I filed bug 411722 about the slowness of js_GetLocalNames.

Comment 3

[Igor Bukanov]

Attachment: v1

The main idea behind this untested patch is to use reserved slots to store arguments and variables of a put call objects. This way js_PutCallObject just needs to copy JSStackFrame.(argv|vars) into the reserved slots delegating the creation of the properties to the resolve hook.

Comment 4

[Igor Bukanov]

Nominating for FF3 as currently js_PutCallObject contributes over 10% to sunspider's date-format-tofte benchmark.

Comment 5

[Mike Schroepfer]

Igor can you comment on regression risk and test coverage for this patch?

Comment 6

[Igor Bukanov]

Attachment: v2

The new version fixes some regressions and optimizes reserved slot initialization in js_PutCallObject.

Comment 7

[Brendan Eich [:brendan]]

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

>+ExtraFrameTrace(JSTracer *trc, JSObject *obj)

Better name: PrivateFrameTrace? Or better, verb at front for this InterCaps static verb-phrase form of naming: TracePrivateFrame or TraceRetainedFrame?

More in a bit, looks good at a glance. Testing going ok?

/be

Comment 8

[Igor Bukanov]

Attachment: v3

The new version fixes short id overflow that previous version of the patch have and uses TracePrivateGenerator as the name of the helper trace function. I hope it does not sound like something from a robbery report about stolen generator. 

The patch passes the test suite for js shell.

Comment 9

[Igor Bukanov]

Attachment: v3 for real

Comment 10

[Igor Bukanov]

Attachment: v4

Fixing bugs that lead to early mochi test failures. Plus the new vesrion no longer removes JSStackFrame.nvars. This can wait another time.

Comment 11

[Mike Schroepfer]

Igor - I'm taking this bug off the blocking list for now.  Once the patch is done and we have understanding of risk/reward we'll reconsider.  At this point we are generally trying to wrap up 1.9 - so this can always come in a follow-on release.

Comment 12

[Brendan Eich [:brendan]]

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

Is the new code on the left (-) side of the diff?

>Index: js/src/jsdbgapi.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsdbgapi.c,v
>retrieving revision 3.139
>diff -U8 -p -r3.139 jsdbgapi.c
>--- js/src/jsdbgapi.c	24 Mar 2008 08:06:39 -0000	3.139
>+++ js/src/jsdbgapi.c	25 Mar 2008 11:22:55 -0000
>@@ -1367,32 +1367,18 @@ JS_GetPropertyDesc(JSContext *cx, JSObje
>         cx->exception = lastException;
>         if (JSVAL_IS_GCTHING(lastException))
>             js_RemoveRoot(cx->runtime, &lastException);
>     }
> 
>     pd->flags |= ((sprop->attrs & JSPROP_ENUMERATE) ? JSPD_ENUMERATE : 0)
>               | ((sprop->attrs & JSPROP_READONLY)  ? JSPD_READONLY  : 0)
>               | ((sprop->attrs & JSPROP_PERMANENT) ? JSPD_PERMANENT : 0)
>-              | ((sprop->getter == js_GetCallVariable) ? JSPD_VARIABLE  : 0);
>-
>-    /* for Call Object 'real' getter isn't passed in to us */

Nit: Capitalize and period at end, of course. But I'm not sure I'm looking at the new code yet, so I'll wait to hear if the diff is reversed.

/be

Comment 13

[Igor Bukanov]

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

The patch requires more work.

Comment 14

[Igor Bukanov]

Attachment: v5

Here is an updated patch without unrelated fixes. The new version stores JSFunction in Call object's slots as JSVAL_OBJECT, not JSVAL_PRIVATE, since with the bug 423874 landed JSFunction became a fat object.

Comment 15

[Igor Bukanov]

Attachment: v6

Here is a patch update to reflect changes due to landing of bug 427798.

Comment 16

[Brendan Eich [:brendan]]

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

Still looks good (I thought I reviewed an earlier version).

/be

Comment 17

[Igor Bukanov]

Attachment: v6 for real

The previous attachment had the wrong patch: that was the fix already checked-in under bug 427798. So here is the right one.

Comment 18

[Brendan Eich [:brendan]]

I thought that patch looked like one that landed already!

I'll review tonight or tomorrow.

/be

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 325398 [details] [diff] [review]
v6 for real

>+#define JSSLOT_SCRIPTED_FUNCTION         (JSSLOT_PRIVATE + 1)
>+#define JSSLOT_PUT_ARGS                  (JSSLOT_PRIVATE + 2)
>+#define CALL_CLASS_FIXED_RESERVED_SLOTS  2

Wondering if PUT_ARGS is best name -- CALL_ARGUMENTS instead? Not really a nit, but the terseness compared to SCRIPTED_FUNCTION, and the PUT possibly being significant, make me ask.

>+     * Since for a call object all fixed slots are taken, we can copy
>+     * arguments and variables straight into JSObject.dslots.

s/are taken/happen to be taken/

>      */
>+    JS_STATIC_ASSERT(JS_INITIAL_NSLOTS ==
>+                     JSSLOT_PRIVATE + 1 + CALL_CLASS_FIXED_RESERVED_SLOTS);

Nit: the 1 in the middle seems odd -- want it at the end, or else subtract it from JS_INITIAL_NSLOTS to show fencepost-limit to maximum-value adjustment.

>+        return setter
>+            ? JS_SetReservedSlot(cx, obj, i, *vp)
>+            : JS_GetReservedSlot(cx, obj, i, vp);

Nit: underhang ? and : below s in setter.

This looks great otherwise -- sorry for the delayed review.

/be

Comment 20

[Igor Bukanov]

Attachment: v6b

The new version addresses the nits from the comment 19.

Comment 21

[Igor Bukanov]

I pushed the patch from the comment 20 to mozilla-central:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/91aa2d65f92d