Closed Bug 411575 Opened 16 years ago Closed 16 years ago

js_PutCallObject() is slow.

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: jst, Assigned: igor)

References

Details

(Keywords: perf)

Attachments

(2 files, 7 obsolete files)

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.
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
Blocks: 411722
I filed bug 411722 about the slowness of js_GetLocalNames.
Assignee: igor → general
No longer blocks: 411722
Depends on: 411722
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: general → igor
Status: NEW → ASSIGNED
Nominating for FF3 as currently js_PutCallObject contributes over 10% to sunspider's date-format-tofte benchmark.
Flags: blocking1.9?
Igor can you comment on regression risk and test coverage for this patch?
Attached patch v2 (obsolete) — Splinter Review
The new version fixes some regressions and optimizes reserved slot initialization in js_PutCallObject.
Attachment #311437 - Attachment is obsolete: true
Attachment #311473 - Flags: review?(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
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #311473 - Attachment is obsolete: true
Attachment #311530 - Flags: review?(brendan)
Attachment #311473 - Flags: review?(brendan)
Attached patch v3 for real (obsolete) — Splinter Review
Attachment #311530 - Attachment is obsolete: true
Attachment #311531 - Flags: review?(brendan)
Attachment #311530 - Flags: review?(brendan)
Attached patch v4 (obsolete) — Splinter Review
Fixing bugs that lead to early mochi test failures. Plus the new vesrion no longer removes JSStackFrame.nvars. This can wait another time.
Attachment #311531 - Attachment is obsolete: true
Attachment #311558 - Flags: review?(brendan)
Attachment #311531 - Flags: review?(brendan)
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.
Flags: blocking1.9? → blocking1.9-
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 on attachment 311558 [details] [diff] [review]
v4

The patch requires more work.
Attachment #311558 - Flags: review?(brendan)
Attachment #311558 - Attachment is obsolete: true
Attached patch v5 (obsolete) — Splinter Review
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.
Attachment #313052 - Flags: review?(brendan)
Blocks: 427798
No longer blocks: 427798
Depends on: 427798
Attached patch v6 (obsolete) — Splinter Review
Here is a patch update to reflect changes due to landing of bug 427798.
Attachment #313052 - Attachment is obsolete: true
Attachment #319606 - Flags: review?(brendan)
Attachment #313052 - Flags: review?(brendan)
Comment on attachment 319606 [details] [diff] [review]
v6

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

/be
Attachment #319606 - Flags: review?(brendan) → review+
Flags: wanted1.9.1?
Attached patch v6 for realSplinter Review
The previous attachment had the wrong patch: that was the fix already checked-in under bug 427798. So here is the right one.
Attachment #319606 - Attachment is obsolete: true
Attachment #325398 - Flags: review?(brendan)
I thought that patch looked like one that landed already!

I'll review tonight or tomorrow.

/be
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P2
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
Attachment #325398 - Flags: review?(brendan) → review+
Attached patch v6bSplinter Review
The new version addresses the nits from the comment 19.
Attachment #327424 - Flags: review+
I pushed the patch from the comment 20 to mozilla-central:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/91aa2d65f92d
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Flags: in-testsuite-
Flags: in-litmus-
Depends on: 488842
No longer depends on: 488842
Depends on: 488842
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: