Closed Bug 716068 Opened 8 years ago Closed 8 years ago

de-OptimizeSpanDeps

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch rm span deps WIP 1 (obsolete) — Splinter Review
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).
Duplicate of this bug: 715356
Attached patch rm span depsSplinter Review
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.
Assignee: general → luke
Attachment #586562 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #586626 - Flags: review?(jwalden+bmo)
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.
Attachment #586626 - Flags: review?(jwalden+bmo) → review+
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
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/addfdfd36160
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.