838813 - Remove source notes no longer necessary due to e4x/decompiler removal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

jorendorff has dibs on this (see bug 836949 comment 5).

Comment 1

[Nicholas Nethercote [inactive]]

BTW, this should reduce the "script-data" entries in about:memory.

Comment 2

[Luke Wagner [:luke]]

May I humbly suggest that you start with the destructuring srcnotes, a particular favorite of mine.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Remove SRC_INITPROP.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Remove SRC_GENEXP.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - Remove SRC_DECL.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - Remove SRC_GROUPASSIGN.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - Remove SRC_DESTRUCT.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: part 6 - Remove SRC_BRACE.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: part 7 - Remove SRC_PCBASE.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 8 - Remove SRC_LABEL and SRC_LABELBRACE.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: part 9 - Remove SRC_ENDBRACE.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: part 10 - Do not remove SRC_SWITCH. Note where it's used.

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: part 11 - Remove SRC_FUNCDEF.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: part 12 - Remove SRC_DESTRUCTLET.

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 710999 [details] [diff] [review]
part 3 - Remove SRC_DECL.

Review of attachment 710999 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ -4248,5 @@
> -        return false;
> -
> -    ptrdiff_t o = PackLetData((bodyEnd - bodyBegin) -
> -                              (JSOP_ENTERLET0_LENGTH + JSOP_LEAVEBLOCK_LENGTH),
> -                              letNotes.isGroupAssign());

I think PackLetData is dead now.  Maybe you'll get it in a later patch?

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 711003 [details] [diff] [review]
part 6 - Remove SRC_BRACE.

Review of attachment 711003 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ -4255,5 @@
> -        pn->expr()->getKind() != PNK_CATCH &&
> -        (stmtInfo.down
> -         ? stmtInfo.down->type == STMT_BLOCK &&
> -           (!stmtInfo.down->down || stmtInfo.down->down->type != STMT_FOR_IN_LOOP)
> -         : !bce->sc->isFunctionBox()))

Good riddance.

::: js/src/frontend/ParseNode.h
@@ +638,4 @@
>                                             which is left kid of PNK_FOR */
>  #define PNX_ENDCOMMA    0x10            /* array literal has comma at end */
>  #define PNX_GROUPINIT   0x20            /* var [a, b] = [c, d]; unit list */
> +#define PNX_FUNCDEFS    0x40            /* contains top-level function statements */

Dude, you just obsoleted my patch in bug 836949!  You know, the one you haven't reviewed yet! :P

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 711014 [details] [diff] [review]
part 8 - Remove SRC_LABEL and SRC_LABELBRACE.

Review of attachment 711014 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this as lots of little patches.  It makes the reviewing much easier.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 711017 [details] [diff] [review]
part 10 - Do not remove SRC_SWITCH. Note where it's used.

Review of attachment 711017 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand this, but just adding an assertion seems fine!

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 711038 [details] [diff] [review]
part 11 - Remove SRC_FUNCDEF.

Review of attachment 711038 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ -4554,5 @@
> -        /*
> -         * This second pass is needed to emit JSOP_NOP with a source note
> -         * for the already-emitted function definition prolog opcode. See
> -         * comments in EmitStatementList.
> -         */

That comment has baffled me for a long time.  What's the second pass?  Does it still exist?

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 711039 [details] [diff] [review]
part 12 - Remove SRC_DESTRUCTLET.

Review of attachment 711039 [details] [diff] [review]:
-----------------------------------------------------------------

Whoa.

Comment 21

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #15)
> Comment on attachment 710999 [details] [diff] [review]
> part 3 - Remove SRC_DECL.
> 
> Review of attachment 710999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ -4248,5 @@
> > -        return false;
> > -
> > -    ptrdiff_t o = PackLetData((bodyEnd - bodyBegin) -
> > -                              (JSOP_ENTERLET0_LENGTH + JSOP_LEAVEBLOCK_LENGTH),
> > -                              letNotes.isGroupAssign());
> 
> I think PackLetData is dead now.  Maybe you'll get it in a later patch?

No, I missed it. Gone in my local repo now...

Comment 22

[Nicholas Nethercote [inactive]]

Some final comments.

- Measurements of the "script-data" values in about:memory would be good.  You can get them on a per-compartment basis, or the total is down the bottom in the "other measurements" section.

- Have you grepped for "decompil"?  That might find other stuff to remove.  E.g. the comment above the definition of SrcNoteType has one occurrence.  And a comment in jsinterp.cpp (line 948) suggests that JSOP_SETNAME and JSOP_SETPROP might be able to be merged:  "Same for JSOP_SETNAME and JSOP_SETPROP, which differ only slightly but remain distinct for the decompiler".

- Is it worth renumbering the remaining srcnotes?  (If so, the XDR version number needs bumping.)

- Is SRC_HIDDEN still needed?  Its comment says "opcode shouldn't be decompiled", but it seems to have other, remaining uses.

Comment 23

[Nicholas Nethercote [inactive]]

> - Measurements of the "script-data" values in about:memory would be good. 

Gmail (if you have an account) and TechCrunch are two sites I often use for measuring, because they are both pigs.  (Gmail more understandably.)

Comment 24

[Jason Orendorff [:jorendorff]]

(In reply to Nicholas Nethercote [:njn] from comment #22)
> - Measurements of the "script-data" values in about:memory would be good. 
> You can get them on a per-compartment basis, or the total is down the bottom
> in the "other measurements" section.

Yes, that would be good... it's probably a miniscule savings, though.

> - Have you grepped for "decompil"?  That might find other stuff to remove. 
> E.g. the comment above the definition of SrcNoteType has one occurrence. 
> And a comment in jsinterp.cpp (line 948) suggests that JSOP_SETNAME and
> JSOP_SETPROP might be able to be merged:  "Same for JSOP_SETNAME and
> JSOP_SETPROP, which differ only slightly but remain distinct for the
> decompiler".

Good point. There is a ton more to do.

> - Is it worth renumbering the remaining srcnotes?

Not to me.

> - Is SRC_HIDDEN still needed?  Its comment says "opcode shouldn't be
> decompiled", but it seems to have other, remaining uses.

Ooh. Yes, SRC_HIDDEN is still needed for two analyses in jsopcode. GetBlockChainAtPC is easy to read, and you can see how it's greatly simplified by SRC_HIDDEN.

But we are emitting it more than we really need to, and the name "SRC_HIDDEN" and the comment have become ridiculous. Filed follow-up bug 839110.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f09dbae62b27
https://hg.mozilla.org/mozilla-central/rev/cb6e7fd832a6
https://hg.mozilla.org/mozilla-central/rev/a09cbd4863fd
https://hg.mozilla.org/mozilla-central/rev/23e605cc0324
https://hg.mozilla.org/mozilla-central/rev/5324e333fcbe
https://hg.mozilla.org/mozilla-central/rev/128396022776
https://hg.mozilla.org/mozilla-central/rev/aaec15131513
https://hg.mozilla.org/mozilla-central/rev/cb83e23119cd
https://hg.mozilla.org/mozilla-central/rev/1da6a150665b
https://hg.mozilla.org/mozilla-central/rev/d001bede8d6a
https://hg.mozilla.org/mozilla-central/rev/489ae4d134aa
https://hg.mozilla.org/mozilla-central/rev/63d7d58ea536