The attached WIP patch removes all traces of span deps by making all jump offsets 4 bytes. It is very simple. Diffstats shows: 20 files changed, 387 insertions(+), 1677 deletions(-) I just hit a bug in OptimizeSpanDeps (bug 715356) for a seemingly unrelated change and it sounds like most other JS engine devs have had the same unpleasant experience. Thus, there is a significant on-going maintenance burden from this 1200 lines of code that must be justified. I measured two things in the browser: js-total-scripts savings explicit (trunk) Trunk Patch Single tab (google.com): 3.63MB 3.70MB 0.07MB 50.3MB Membuster: 232.16MB 236.90MB 4.74MB 1991.18MB Thus, the spandeps optimization provides a 0.13% - 0.23% total browser improvement. Given that there is an ongoing maintenance overhead, we have to ask ourselves (as dmandelin put it): would we accept this as a new patch today? Seems like 'no'. Nick, from a MemShrink perspective, do you agree? I still have some cleanup to do in the patch (like removing the trailing X from everything).

So far green on try. This patch leaves JSOP_UNUSED0-13. Instead of churning jsopcode.tbl, I'll file a new bug to do the work since it could use a general defragmentation.

Sounds ok to me.

Also, any extra memory consumed would be gained back by lazy compilation of scripts to bytecode.

(In reply to Brian Hackett (:bhackett) from comment #4) > Also, any extra memory consumed would be gained back by lazy compilation of > scripts to bytecode. Sounds like we have a volunteer! :P

Comment on attachment 586626 [details] [diff] [review] rm span deps Review of attachment 586626 [details] [diff] [review]: ----------------------------------------------------------------- "A man's got to know his limitations." "When someone asks you if you're a god, you say 'YES'!" ::: js/src/frontend/BytecodeEmitter.cpp @@ +769,5 @@ > { > StmtInfo *stmt = bce->topStmt; > if (!STMT_IS_TRYING(stmt) && > (!BackPatch(cx, bce, stmt->breaks, bce->next(), JSOP_GOTO) || > + !BackPatch(cx, bce, stmt->continues, bce->code(stmt->update), JSOP_GOTO))) { { on next line. ::: js/src/jsinterp.cpp @@ +1999,5 @@ > { > len = GET_JUMP_OFFSET(regs.pc); > BRANCH(len); > } > +END_CASE(JSOP_GOTO); Erm, why are you changing this? The other end-cases I see in context don't have this. @@ +2042,5 @@ > { > bool cond; > Value *_; > VALUE_TO_BOOLEAN(cx, _, cond); > + if (cond == JS_FALSE) { This doesn't look right either. @@ +3591,2 @@ > /* > * JSOP_LOOKUPSWITCH and JSOP_LOOKUPSWITCHX are never used if any atom vestigial ::: js/src/jsopcode.cpp @@ +2361,2 @@ > } > + LOCAL_ASSERT(tail + GET_JUMP_OFFSET(pc+tail) == pc2 - pc); Spaces around that +. @@ +2909,2 @@ > /* > + * JSOP_GOSUB have no effect on the decompiler's string stack has @@ +5889,5 @@ > > /* > * A (C ? T : E) expression requires skipping either T (if target is in > * E) or both T and E (if target is after the whole expression) before > + * adjusting pcdepth based on the JSOP_IFEQ or JSOP_IFEQ at pc that ... ::: js/src/jsopcode.h @@ +165,5 @@ > +#define JUMP_OFFSET_LEN 4 > +#define JUMP_OFFSET_B3(off) ((jsbytecode)((off) >> 24)) > +#define JUMP_OFFSET_B2(off) ((jsbytecode)((off) >> 16)) > +#define JUMP_OFFSET_B1(off) ((jsbytecode)((off) >> 8)) > +#define JUMP_OFFSET_B0(off) ((jsbytecode)(off)) Please remove the B3..0 macros. They're only used once, and if the GET macro is structurally similar to the SET macro, I don't think these help. @@ +171,2 @@ > | ((pc)[3] << 8) | (pc)[4])) > +#define SET_JUMP_OFFSET(pc,off)((pc)[1] = JUMP_OFFSET_B3(off), \ ...or is it because off might have the wrong type that those macros were used? Make both the GET macro and the SET macro into properly typed inline functions to solve that, and to improve readability. @@ +173,5 @@ > + (pc)[2] = JUMP_OFFSET_B2(off), \ > + (pc)[3] = JUMP_OFFSET_B1(off), \ > + (pc)[4] = JUMP_OFFSET_B0(off)) > +#define JUMP_OFFSET_MIN (int32_t(0x80000000)) > +#define JUMP_OFFSET_MAX (int32_t(0x7fffffff)) INT32_MIN and INT32_MAX.

Great to see this old code go. It was from simpler days. No JITs, most scripts with jumps much smaller than today, etc. /be

(In reply to Jeff Walden (remove +bmo to email) from comment #6) Thanks Waldo; all good changes. https://hg.mozilla.org/integration/mozilla-inbound/rev/addfdfd36160