Last Comment Bug 838813 - Remove source notes no longer necessary due to e4x/decompiler removal
: Remove source notes no longer necessary due to e4x/decompiler removal
Status: RESOLVED FIXED
[js:t][MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: Jason Orendorff [:jorendorff]
: general
Mentors:
Depends on:
Blocks: 838960
  Show dependency treegraph
 
Reported: 2013-02-06 13:31 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2013-02-09 07:48 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Remove SRC_INITPROP. (2.34 KB, patch)
2013-02-06 15:05 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 2 - Remove SRC_GENEXP. (1.61 KB, patch)
2013-02-06 15:07 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 3 - Remove SRC_DECL. (5.02 KB, patch)
2013-02-06 15:08 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 4 - Remove SRC_GROUPASSIGN. (1.46 KB, patch)
2013-02-06 15:09 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 5 - Remove SRC_DESTRUCT. (1.54 KB, patch)
2013-02-06 15:10 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 6 - Remove SRC_BRACE. (6.04 KB, patch)
2013-02-06 15:12 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 7 - Remove SRC_PCBASE. (6.96 KB, patch)
2013-02-06 15:13 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 8 - Remove SRC_LABEL and SRC_LABELBRACE. (3.92 KB, patch)
2013-02-06 15:31 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 9 - Remove SRC_ENDBRACE. (2.06 KB, patch)
2013-02-06 15:35 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 10 - Do not remove SRC_SWITCH. Note where it's used. (636 bytes, patch)
2013-02-06 15:37 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 11 - Remove SRC_FUNCDEF. (3.34 KB, patch)
2013-02-06 15:58 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review
part 12 - Remove SRC_DESTRUCTLET. (10.85 KB, patch)
2013-02-06 15:59 PST, Jason Orendorff [:jorendorff]
n.nethercote: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-06 13:31:46 PST
jorendorff has dibs on this (see bug 836949 comment 5).
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-06 13:32:52 PST
BTW, this should reduce the "script-data" entries in about:memory.
Comment 2 Luke Wagner [:luke] 2013-02-06 13:40:43 PST
May I humbly suggest that you start with the destructuring srcnotes, a particular favorite of mine.
Comment 3 Jason Orendorff [:jorendorff] 2013-02-06 15:05:56 PST
Created attachment 710997 [details] [diff] [review]
part 1 - Remove SRC_INITPROP.
Comment 4 Jason Orendorff [:jorendorff] 2013-02-06 15:07:21 PST
Created attachment 710998 [details] [diff] [review]
part 2 - Remove SRC_GENEXP.
Comment 5 Jason Orendorff [:jorendorff] 2013-02-06 15:08:18 PST
Created attachment 710999 [details] [diff] [review]
part 3 - Remove SRC_DECL.
Comment 6 Jason Orendorff [:jorendorff] 2013-02-06 15:09:30 PST
Created attachment 711000 [details] [diff] [review]
part 4 - Remove SRC_GROUPASSIGN.
Comment 7 Jason Orendorff [:jorendorff] 2013-02-06 15:10:49 PST
Created attachment 711001 [details] [diff] [review]
part 5 - Remove SRC_DESTRUCT.
Comment 8 Jason Orendorff [:jorendorff] 2013-02-06 15:12:28 PST
Created attachment 711003 [details] [diff] [review]
part 6 - Remove SRC_BRACE.
Comment 9 Jason Orendorff [:jorendorff] 2013-02-06 15:13:59 PST
Created attachment 711004 [details] [diff] [review]
part 7 - Remove SRC_PCBASE.
Comment 10 Jason Orendorff [:jorendorff] 2013-02-06 15:31:44 PST
Created attachment 711014 [details] [diff] [review]
part 8 - Remove SRC_LABEL and SRC_LABELBRACE.
Comment 11 Jason Orendorff [:jorendorff] 2013-02-06 15:35:31 PST
Created attachment 711015 [details] [diff] [review]
part 9 - Remove SRC_ENDBRACE.
Comment 12 Jason Orendorff [:jorendorff] 2013-02-06 15:37:12 PST
Created attachment 711017 [details] [diff] [review]
part 10 - Do not remove SRC_SWITCH. Note where it's used.
Comment 13 Jason Orendorff [:jorendorff] 2013-02-06 15:58:16 PST
Created attachment 711038 [details] [diff] [review]
part 11 - Remove SRC_FUNCDEF.
Comment 14 Jason Orendorff [:jorendorff] 2013-02-06 15:59:54 PST
Created attachment 711039 [details] [diff] [review]
part 12 - Remove SRC_DESTRUCTLET.
Comment 15 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-06 21:04:24 PST
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 [:njn] (on vacation until July 11) 2013-02-06 21:10:18 PST
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 [:njn] (on vacation until July 11) 2013-02-06 21:14:01 PST
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 [:njn] (on vacation until July 11) 2013-02-06 21:16:14 PST
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 [:njn] (on vacation until July 11) 2013-02-06 21:18:43 PST
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 [:njn] (on vacation until July 11) 2013-02-06 21:21:36 PST
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] 2013-02-06 21:24:25 PST
(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 [:njn] (on vacation until July 11) 2013-02-06 21:29:25 PST
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 [:njn] (on vacation until July 11) 2013-02-06 21:30:45 PST
> - 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] 2013-02-07 13:18:43 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.