447762 - SM: merging var and local bytecodes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

With the bug 441686 fixed, the interpreter uses the same code for VAR and LOCAL bytecode pairs. These pairs can be replaced with single bytecodes if the decompile learns how to distinguish a variable from a local based on its slot number.

Comment 1

[Igor Bukanov]

Attachment: work in progress

This patch compiles but untested. With the patch there is no longer any VAR bytecodes, all of them are merged into the corresponding LOCAL version, which eliminated 9 bytecodes in total.

Comment 2

[Igor Bukanov]

Attachment: v1

The new version of the patch passes shell and mochi tests. Most of the changes are in the decompiler to distinguish local from var index based on the slot number.

Comment 3

[Igor Bukanov]

Attachment: v2

I forgot in v1 to bump the XDR version, the new version fixes that.

Comment 4

[Brendan Eich [:brendan]]

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

>+static jsint
>+ReadSlot(JSPrinter *jp, jsbytecode *pc, JSSlotKind *kindp)
>+{
>+    jsint i;
>+
>+    i = GET_SLOTNO(pc);
>+    if (i < jp->script->nfixed) {
>+        *kindp = JSSK_VAR;
>+        i += jp->fun->nargs;
>+    } else {
>+        *kindp = JSSK_LOCAL;
>+        i -= jp->script->nfixed;
>+    }
>+    return i;
>+}

Hard to improve on ReadSlot, yet it seems missnamed. Get is the usual verb, whether decoding an immediate from pc or getting an atom from an atomMap, etc. GetSlotAndKind is too long, though. Not really commenting usefully here, just ruminating in case you get a better idea from this.

On jsopcode.tbl changes:

> /* ECMA-compliant for-in loop with argument or local variable loop control. */
> OPDEF(JSOP_FORARG,    10, "forarg",     NULL,         3,  0,  1, 19,  JOF_QARG|JOF_NAME|JOF_FOR)
>-OPDEF(JSOP_FORVAR,    11, "forvar",     NULL,         3,  0,  1, 19,  JOF_QVAR|JOF_NAME|JOF_FOR)
>+OPDEF(JSOP_FORLOCAL,  11,"forlocal",    NULL,         3,  0,  1, 19,  JOF_LOCAL|JOF_NAME|JOF_FOR)

Please keep columns aligned, in this case the "forlocal" opname needs a space before and take away one after. Same goes for later.

We will need to compress and reorder the table in one big patch, so these kinds of patches could just set ops to UNUSED status and leave others where they were -- but I'm in favor of this patch being non-minimal in that sense, to get the LOCAL ops down where the VAR ones were.

r=me with nits picked.

/be

Comment 5

[Igor Bukanov]

Attachment: v3

The new version of the patch fixes the nits and adds comments. I changed ReadSlot into IsVarSlot:

-static jsint
-ReadSlot(JSPrinter *jp, jsbytecode *pc, JSSlotKind *kindp)
+static JSBool
+IsVarSlot(JSPrinter *jp, jsbytecode *pc, jsint *indexp)

This emphasis that the main purpose of the function is to decided whether the slot is var or local. The index is just a bonus. The change allowed to eliminate that slotKind local variable and made var/let ifs less noisy.

Comment 6

[Igor Bukanov]

Attachment: v3b

The new version gives better comments to IsVarSlot.

Comment 7

[Igor Bukanov]

landed - http://hg.mozilla.org/index.cgi/mozilla-central/rev/439726ab7e33