427798 - js_PutBlockObject() is slow.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #411575 +++

The bug 411575 exposed that js_PutCallObject is slow because it creates properties representing variables and argument names. This is an expensive operation and the proposed solution in the bug is to create the properties lazily while using the reserved slots to store the value of frame variable and arguments on the moment of frame exit.

Since js_PutBlockObject has exactly the same structure as the problematic js_PutCallObject, I suggest to optimize in the same way as the proposed solution for js_PutCallObject. That is, the block object should use the reserved slots to store the values of the let variables after the code leaves the let block.

Comment 1

[Igor Bukanov]

Recording fix order code dependencies.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking blocking since it blocks a blocker.

Comment 3

[Igor Bukanov]

Reversing patch dependency: bug 411575 is not 1.9 blocker and can be fixed after fixing this one.

Comment 4

[Igor Bukanov]

Attachment: nackup of something that compiles

This patch is build on top of the patch from bug 389605 comment 14.

Comment 5

[Igor Bukanov]

Attachment: regression and performance test

I am getting errors about reserved slot beyond the range with this test. It seems like a bug in JSOP_SETNAME/JSOP_SETPROP implementation.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What's the latest status here? This is still blocking a relatively leak-prone blocker.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> What's the latest status here? This is still blocking a relatively leak-prone
> blocker.

The patch here needs a fix for bug 428282.

Comment 8

[Igor Bukanov]

Attachment: v1

This what I will test before asking for a review.

Comment 9

[Igor Bukanov]

Comment on attachment 318117 [details] [diff] [review]
v1

Note that the reason the patch renames ReallocSlots into js_ReallocSlots is to minimize the patch for bug 411575. It will need to call the function from jsfun.h.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> (From update of attachment 318117 [details] [diff] [review])
> Note that the reason the patch renames ReallocSlots into js_ReallocSlots is to
> minimize the patch for bug 411575. It will need to call the function from
> jsfun.h.

I meant from "jsfun.c"

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 318117 [details] [diff] [review]
v1

>+    /* For a block object we have one available slot among fixed slots. */
>+    JS_STATIC_ASSERT(JS_INITIAL_NSLOTS == JSSLOT_BLOCK_DEPTH + 2);

Comment might rather say "Blocks have one fixed slot available for the first local."

>+    /*
>+     * Block objects should never be exposed to scripts. Thus the clone should
>+     * not own the property map and rather always share it with the original

s/original/prototype/

>+     * object. It allows to skip updating OBJ_SCOPE(obj)->map.freeslot after

s/It allows/This allows us/

>+     * we copy below the stack slots into reserved slots.

s/below//

>+    /* Block objects should not have reserve slots before they are put. */

s/reserve slots/reserved slots/

>+    JS_ASSERT(STOBJ_NSLOTS(obj) == JS_INITIAL_NSLOTS);
>+
>+    /* Block and its locals must be on the current stack for GC safety. */

s/Block/The block/

>+    /* Reserve slots counts from the first slot after the private. */

s/Reserve slots counts from/Reserved slots start with/

Same below in block_setProperty.

>@@ -2884,17 +2905,17 @@ js_FreeSlot(JSContext *cx, JSObject *obj
> 
>     map = obj->map;
>     JS_ASSERT(!MAP_IS_NATIVE(map) || ((JSScope *)map)->object == obj);
>     LOCKED_OBJ_SET_SLOT(obj, slot, JSVAL_VOID);
>     if (map->freeslot == slot + 1) {
>         map->freeslot = slot;
> 
>         /* When shrinking ReallocSlots always returns true. */
>-        ReallocSlots(cx, obj, slot, JS_FALSE);
>+        js_ReallocSlots(cx, obj, slot, JS_FALSE);

Existing comemnt should say js_ReallocSlots -- would be nice to add a comma before that (after "shrinking") too.

r=me with nits picked. Thanks,

/be

Comment 12

[Igor Bukanov]

Attachment: v1b

The new version of the patch addresses the nit from the comment 11.

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 318872 [details] [diff] [review]
v1b

a=shaver. (Approving last JS blocker? I have lived my whole life for this day.)

Comment 14

[Igor Bukanov]

I checked in the patch from the comment 12 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1209679099&maxdate=1209679200&who=igor%25mir2.org

Comment 15

[Mike Schroepfer]

Bug 432172 possibly caused by this?

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> Bug 432172 possibly caused by this?

The patch affects only the code that defines closures or calls exec in a block with let variables. Moreover, in such cases it should save memory as the patch uses shared properties of the let block object that allows to skip creation of properties when the code leaves the block.

So in theory the patch can not be responsible for the malloc jump. Of cause due to a bug the patch can cause a jump in malloc and to check this I can backout the patch. 

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/regress/regress-427798.js,v  <--  regress-427798.js
initial revision: 1.1

mozilla-central: 16423:d516f037d2a6