346019 - Unable to XDR let statements

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Sicking noticed the other day that we are unable to XDR let statements due to the block object.

I'm working on fixing this.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This works for my simple testcases. I think the changes to add xdr_blockObject don't require any special explanation. I had to add a cached class object for Block, since without it, js_XDRObject has no way (when decoding) to know what clasp to call xdrObject on. This brought up an interesting problem: we can't #ifdef out stuff entirely in jsproto.tbl, since if we do, then the indices (2nd argument) don't line up with the arrays up to JSProto_LIMIT that we create, and we get crashes. Instead, this patch uses js_InitNullClass for things initialization functions that aren't configured on.

I also had to change the decompiler to not care about the order that the properties were defined on the block object.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1.1

Oops, the loop in the decompiler still needed some tweaking.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 230980 [details] [diff] [review]
patch v1.1

>+jsatomid
>+FindObjectAtomIndex(JSAtomMap *map, JSObject *obj)
>+{
>+    size_t i;
>+    JSAtom *atom;
>+
>+    for (i = 0; i < map->length; i++) {
>+        atom = map->vector[i];
>+        if (ATOM_IS_OBJECT(atom) && ATOM_TO_OBJECT(atom) == obj)
>+            return atom->number;
>+    }
>+
>+    return NO_PARENT_INDEX;
>+}

This could get ugly -- O(M*N) for M blocks and N atoms on average skipped during the search.  On this account and to reduce code size, I again think an xdr->blockChain is the way to go.  No parentId needed.

>+            /* Find a property to XDR. */
>+            do {
>+                /* If sprop is NULL, this is the first property. */
>+                sprop = sprop ? sprop->parent : OBJ_SCOPE(*objp)->lastProp;

The ?: should be elinated by setting sprop = lastProp before the loop and using a while loop.

>+        /* XDR the real id, then the shortid. */
>+        if (!js_XDRStringAtom(xdr, &atom) ||
>+            !JS_XDRUint16(xdr, (uint16 *)&shortid)) {

I wonder whether here and earlier for depth and count, you might do better to use a uint32 tmp as an intermediary. That would better show the future-proofing inherent in XDR's alignment to int (long, 32-bit) word boundary.

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1.5

This patch actually works for the nested case. It still has the scary O(n*m) growth rate for finding block parents, but we can fix that by culling the atom map early if it turns out to be a problem.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 231502 [details] [diff] [review]
Updated to comments

Looks good, thanks. Another fix for JS1.7 to make the language sane.  Without this fix, chrome code that's fastloaded can't use let statements.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 8

[Brendan Eich [:brendan]]

Attachment: followup patch to avoid a gcc warning, and add an XXX comment

I am checking this into the trunk.  It should follow the main patch onto the 1.8 branch.

/be

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231502 [details] [diff] [review]
Updated to comments

a=drivers, please land this on the MOZILLA_1_8_BRANCH.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231535 [details] [diff] [review]
followup patch to avoid a gcc warning, and add an XXX comment

This too.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch.