Closed
Bug 842419
Opened 12 years ago
Closed 12 years ago
Clean up some front-end cruft
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [js:t])
Attachments
(8 files, 4 obsolete files)
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
I have various patches coming shortly.
Assignee | ||
Comment 2•12 years ago
|
||
The only remaining use of SRC_CONTINUE is IonMonkey, and it only needs it on JSOP_GOTO.
Attachment #715285 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•12 years ago
|
||
We forgot to update this in bug 838813. I also numbered the elements for easier reading, and used "foo/bar" for the cases where the srcnote numbers overlap.
Attachment #715290 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•12 years ago
|
||
I forgot to update the SRC_CONTINUE comment.
Attachment #715297 -
Flags: review?(jorendorff)
Assignee | ||
Updated•12 years ago
|
Attachment #715285 -
Attachment is obsolete: true
Attachment #715285 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 715290 [details] [diff] [review] (part 4) - Fix up js_SrcNoteSpec[]. I have a better version of this coming up.
Attachment #715290 -
Attachment is obsolete: true
Attachment #715290 -
Flags: review?(jorendorff)
Assignee | ||
Comment 7•12 years ago
|
||
This patch: - Defines the source note values in a more sensible order, grouping related ones together. - Stops the sharing of some of the values, which is just confusing. - Numbers the entries in js_SrcNoteSpec[] for easier reading. - Makes SN_IS_GETTABLE more robust. There are now 4 unused values before SRC_XDELTA. I tried changing SRC_XDELTA from 24 to 20 but this causes all sorts of test failures, and I haven't worked out why. Mildly terrifying...
Attachment #715320 -
Flags: review?(jorendorff)
Assignee | ||
Comment 8•12 years ago
|
||
SRC_IF_ELSE's first operand is used by IonMonkey. It's second isn't used.
Attachment #715324 -
Flags: review?(jorendorff)
Assignee | ||
Updated•12 years ago
|
Attachment #715324 -
Attachment is obsolete: true
Attachment #715324 -
Flags: review?(jorendorff)
Assignee | ||
Comment 10•12 years ago
|
||
This patch reduces the arity of four loop-related srcnotes. This avoids emitting some atom indices, which is nice. Note that SRC_FOR_IN's 2nd offset has become its 1st, and so its layout matches SRC_WHILE's layout -- that explains the change in whileOrForInLoop().
Attachment #715332 -
Flags: review?(jorendorff)
Assignee | ||
Comment 11•12 years ago
|
||
> There are now 4 unused values before SRC_XDELTA. I tried changing SRC_XDELTA
> from 24 to 20 but this causes all sorts of test failures, and I haven't
> worked out why. Mildly terrifying...
Oh, now I get it. 24 is 11000b, and it's XDELTA if the top 2 bits are set.
Assignee | ||
Comment 12•12 years ago
|
||
TABLESWITCH only needs 1 offset, but CONDSWITCH needs 2.
Attachment #715463 -
Flags: review?(jorendorff)
Assignee | ||
Comment 14•12 years ago
|
||
Oh, we don't need SRC_CATCH on JSOP_ENTERBLOCK.
Attachment #715867 -
Flags: review?(jorendorff)
Assignee | ||
Updated•12 years ago
|
Attachment #715828 -
Attachment is obsolete: true
Attachment #715828 -
Flags: review?(jorendorff)
Updated•12 years ago
|
Attachment #715282 -
Flags: review?(jorendorff) → review+
Updated•12 years ago
|
Attachment #715297 -
Flags: review?(jorendorff) → review+
Updated•12 years ago
|
Attachment #715288 -
Flags: review?(jorendorff) → review+
Comment 15•12 years ago
|
||
Comment on attachment 715320 [details] [diff] [review] (part 4) - Clean up srcnote constants and js_SrcNoteSpec. Review of attachment 715320 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.h @@ +247,5 @@ > * enum, so its initializers need to match the order here. > * > + * It's possible for two source note kinds to share the note type value if they > + * have the same arity and cannot both apply to the same opcode. But please > + * don't do that -- it's just confusing. Agreed, but how about we just delete this paragraph? I think all current maintainers are on the same page.
Attachment #715320 -
Flags: review?(jorendorff) → review+
Comment 16•12 years ago
|
||
Comment on attachment 715330 [details] [diff] [review] (part 5) - Reduce the arity of SRC_IF_ELSE from 2 to 1. Review of attachment 715330 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +3905,5 @@ > } else { > /* > * We came here from the goto further below that detects else-if > * chains, so we must mutate stmtInfo back into a STMT_IF record. > + * Also we need a note offset for SRC_IF_ELSE to help IonMonkey. That goto: (Γ¨_Γ©) r=me if you're willing to change it to tail-recurse instead; the relevant unit test is tests/js1_5/Regress/regress-96526-003.js ::: js/src/shell/js.cpp @@ +1747,1 @@ > break; You don't need a Sprintf here if you don't want one, just reuse the one below (case SRC_COND et al). Otherwise: it'll fit on one line.
Attachment #715330 -
Flags: review?(jorendorff) → review+
Comment 17•12 years ago
|
||
Comment on attachment 715332 [details] [diff] [review] (part 6) - Reduce the arity of four loop-related srcnotes. Review of attachment 715332 [details] [diff] [review]: ----------------------------------------------------------------- Yes please! I think SRC_CONT2LABEL is equivalent to SRC_CONTINUE with this change. Please unify! (Unfortunately there is some sort of minor difference between SRC_BREAK and SRC_BREAK2LABEL, but it looks like maybe they could be unified too, with a bit more work.) ::: js/src/frontend/BytecodeEmitter.cpp @@ +649,5 @@ > return -1; > > + if (noteType != SRC_NULL) > + if (NewSrcNote(cx, bce, noteType) < 0) > + return -1; Style nit: Braces on the outer if-statement, please. Is the followup already on file to use opcodes rather than source notes for this? ::: js/src/ion/IonBuilder.cpp @@ +2255,5 @@ > // 3/ Generate code for all bodies (see processCondSwitchBody). > > JS_ASSERT(JSOp(*pc) == JSOP_CONDSWITCH); > jssrcnote *sn = info().getNote(cx, pc); > + JS_ASSERT(SN_TYPE(sn) == SRC_SWITCH); Thank you so much for adding these assertions. It's more than bold going commando to a source-notes dance; it's crazy. What were the original authors thinking?! ;) ::: js/src/shell/js.cpp @@ +1755,1 @@ > break; Super-micro-nit: If you prefer to have these cases explicit here, might as well remove the default:; case and fill in the other missing source notes. Your call.
Attachment #715332 -
Flags: review?(jorendorff) → review+
Comment 18•12 years ago
|
||
Comment on attachment 715463 [details] [diff] [review] (part 7) - Split SRC_SWITCH in two. Review of attachment 715463 [details] [diff] [review]: ----------------------------------------------------------------- Sure, OK. ::: js/src/frontend/BytecodeEmitter.cpp @@ +2329,3 @@ > if (switchOp == JSOP_CONDSWITCH) { > /* > * 0 bytes of immediate for unoptimized ECMAv2 switch. get rid of "ECMAv2" here while you're in the neighborhood?
Attachment #715463 -
Flags: review?(jorendorff) → review+
Comment 19•12 years ago
|
||
Comment on attachment 715867 [details] [diff] [review] (part 8) - Reduce the arity of SRC_CATCH from 1 to 0. Review of attachment 715867 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Now that I know ReconstructPCStack is the only code using SRC_CATCH, I'll take a look and see if we can get rid of it too. ::: js/src/frontend/BytecodeEmitter.cpp @@ +3610,1 @@ > /* ifeq <next block> */ Pre-existing style nit: comments get a blank line before (except at the top of a file or block).
Attachment #715867 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 20•12 years ago
|
||
> Is the followup already on file to use opcodes rather than source notes for
> this?
Not yet. I'm still mulling over exactly how many srcnotes can be handled in this way. But it's on my todo list.
Assignee | ||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01991f5219c https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ad1347a31a https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ff88dac40a https://hg.mozilla.org/integration/mozilla-inbound/rev/75a6fd76e428 https://hg.mozilla.org/integration/mozilla-inbound/rev/57e43753805f https://hg.mozilla.org/integration/mozilla-inbound/rev/88c652c92bc8 https://hg.mozilla.org/integration/mozilla-inbound/rev/c40a568d6929 https://hg.mozilla.org/integration/mozilla-inbound/rev/14a91fda6743
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a01991f5219c https://hg.mozilla.org/mozilla-central/rev/f2ad1347a31a https://hg.mozilla.org/mozilla-central/rev/c7ff88dac40a https://hg.mozilla.org/mozilla-central/rev/75a6fd76e428 https://hg.mozilla.org/mozilla-central/rev/57e43753805f https://hg.mozilla.org/mozilla-central/rev/88c652c92bc8 https://hg.mozilla.org/mozilla-central/rev/c40a568d6929 https://hg.mozilla.org/mozilla-central/rev/14a91fda6743
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•