The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: jorendorff)

Tracking

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t][MemShrink:P2])

Attachments

(12 attachments)

2.34 KB, patch
njn
: review+
Details | Diff | Splinter Review
1.61 KB, patch
njn
: review+
Details | Diff | Splinter Review
5.02 KB, patch
njn
: review+
Details | Diff | Splinter Review
1.46 KB, patch
njn
: review+
Details | Diff | Splinter Review
1.54 KB, patch
njn
: review+
Details | Diff | Splinter Review
6.04 KB, patch
njn
: review+
Details | Diff | Splinter Review
6.96 KB, patch
njn
: review+
Details | Diff | Splinter Review
3.92 KB, patch
njn
: review+
Details | Diff | Splinter Review
2.06 KB, patch
njn
: review+
Details | Diff | Splinter Review
636 bytes, patch
njn
: review+
Details | Diff | Splinter Review
3.34 KB, patch
njn
: review+
Details | Diff | Splinter Review
10.85 KB, patch
njn
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
jorendorff has dibs on this (see bug 836949 comment 5).
(Reporter)

Comment 1

4 years ago
BTW, this should reduce the "script-data" entries in about:memory.
Whiteboard: [js:t] → [js:t][MemShrink]

Comment 2

4 years ago
May I humbly suggest that you start with the destructuring srcnotes, a particular favorite of mine.
(Reporter)

Updated

4 years ago
Whiteboard: [js:t][MemShrink] → [js:t][MemShrink:P2]
(Assignee)

Comment 3

4 years ago
Created attachment 710997 [details] [diff] [review]
part 1 - Remove SRC_INITPROP.
Attachment #710997 - Flags: review?(n.nethercote)
(Assignee)

Comment 4

4 years ago
Created attachment 710998 [details] [diff] [review]
part 2 - Remove SRC_GENEXP.
Attachment #710998 - Flags: review?(n.nethercote)
(Assignee)

Comment 5

4 years ago
Created attachment 710999 [details] [diff] [review]
part 3 - Remove SRC_DECL.
Attachment #710999 - Flags: review?(n.nethercote)
(Assignee)

Updated

4 years ago
Attachment #710997 - Attachment description: , part 1 - Remove SRC_INITPROP. → part 1 - Remove SRC_INITPROP.
(Assignee)

Comment 6

4 years ago
Created attachment 711000 [details] [diff] [review]
part 4 - Remove SRC_GROUPASSIGN.
Attachment #711000 - Flags: review?(n.nethercote)
(Assignee)

Comment 7

4 years ago
Created attachment 711001 [details] [diff] [review]
part 5 - Remove SRC_DESTRUCT.
Attachment #711001 - Flags: review?(n.nethercote)
(Assignee)

Comment 8

4 years ago
Created attachment 711003 [details] [diff] [review]
part 6 - Remove SRC_BRACE.
Attachment #711003 - Flags: review?(n.nethercote)
(Assignee)

Comment 9

4 years ago
Created attachment 711004 [details] [diff] [review]
part 7 - Remove SRC_PCBASE.
Attachment #711004 - Flags: review?(n.nethercote)
(Assignee)

Comment 10

4 years ago
Created attachment 711014 [details] [diff] [review]
part 8 - Remove SRC_LABEL and SRC_LABELBRACE.
Attachment #711014 - Flags: review?(n.nethercote)
(Assignee)

Comment 11

4 years ago
Created attachment 711015 [details] [diff] [review]
part 9 - Remove SRC_ENDBRACE.
Attachment #711015 - Flags: review?(n.nethercote)
(Assignee)

Comment 12

4 years ago
Created attachment 711017 [details] [diff] [review]
part 10 - Do not remove SRC_SWITCH. Note where it's used.
Attachment #711017 - Flags: review?(n.nethercote)
(Assignee)

Comment 13

4 years ago
Created attachment 711038 [details] [diff] [review]
part 11 - Remove SRC_FUNCDEF.
Attachment #711038 - Flags: review?(n.nethercote)
(Assignee)

Comment 14

4 years ago
Created attachment 711039 [details] [diff] [review]
part 12 - Remove SRC_DESTRUCTLET.
Attachment #711039 - Flags: review?(n.nethercote)
(Assignee)

Updated

4 years ago
Blocks: 838960
(Reporter)

Updated

4 years ago
Attachment #710997 - Flags: review?(n.nethercote) → review+
(Reporter)

Updated

4 years ago
Attachment #710998 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 15

4 years ago
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?
Attachment #710999 - Flags: review?(n.nethercote) → review+
(Reporter)

Updated

4 years ago
Attachment #711000 - Flags: review?(n.nethercote) → review+
(Reporter)

Updated

4 years ago
Attachment #711001 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 16

4 years ago
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
Attachment #711003 - Flags: review?(n.nethercote) → review+
(Reporter)

Updated

4 years ago
Attachment #711004 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 17

4 years ago
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.
Attachment #711014 - Flags: review?(n.nethercote) → review+
(Reporter)

Updated

4 years ago
Attachment #711015 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 18

4 years ago
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!
Attachment #711017 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 19

4 years ago
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?
Attachment #711038 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 20

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

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

Whoa.
Attachment #711039 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 21

4 years ago
(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...
(Reporter)

Comment 22

4 years ago
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.
(Reporter)

Comment 23

4 years ago
> - 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.)
(Assignee)

Comment 24

4 years ago
(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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.