Closed
Bug 838813
Opened 12 years ago
Closed 12 years ago
Remove source notes no longer necessary due to e4x/decompiler removal
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: n.nethercote, Assigned: jorendorff)
References
Details
(Whiteboard: [js:t][MemShrink:P2])
Attachments
(12 files)
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
jorendorff has dibs on this (see bug 836949 comment 5).
![]() |
Reporter | |
Comment 1•12 years ago
|
||
BTW, this should reduce the "script-data" entries in about:memory.
Whiteboard: [js:t] → [js:t][MemShrink]
![]() |
||
Comment 2•12 years ago
|
||
May I humbly suggest that you start with the destructuring srcnotes, a particular favorite of mine.
![]() |
Reporter | |
Updated•12 years ago
|
Whiteboard: [js:t][MemShrink] → [js:t][MemShrink:P2]
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #710997 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #710998 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #710999 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•12 years ago
|
Attachment #710997 -
Attachment description: , part 1 - Remove SRC_INITPROP. → part 1 - Remove SRC_INITPROP.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #711000 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #711001 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #711003 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #711004 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #711014 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #711015 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #711017 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #711038 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #711039 -
Flags: review?(n.nethercote)
![]() |
Reporter | |
Updated•12 years ago
|
Attachment #710997 -
Flags: review?(n.nethercote) → review+
![]() |
Reporter | |
Updated•12 years ago
|
Attachment #710998 -
Flags: review?(n.nethercote) → review+
![]() |
Reporter | |
Comment 15•12 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•12 years ago
|
Attachment #711000 -
Flags: review?(n.nethercote) → review+
![]() |
Reporter | |
Updated•12 years ago
|
Attachment #711001 -
Flags: review?(n.nethercote) → review+
![]() |
Reporter | |
Comment 16•12 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•12 years ago
|
Attachment #711004 -
Flags: review?(n.nethercote) → review+
![]() |
Reporter | |
Comment 17•12 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•12 years ago
|
Attachment #711015 -
Flags: review?(n.nethercote) → review+
![]() |
Reporter | |
Comment 18•12 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•12 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•12 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•12 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•12 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•12 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•12 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.
Comment 25•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•