Last Comment Bug 716068 - de-OptimizeSpanDeps
: de-OptimizeSpanDeps
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 715356 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-06 14:06 PST by Luke Wagner [:luke]
Modified: 2012-01-10 01:41 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm span deps WIP 1 (195.77 KB, patch)
2012-01-06 14:06 PST, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
rm span deps (155.19 KB, patch)
2012-01-06 17:24 PST, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-01-06 14:06:44 PST
Created attachment 586562 [details] [diff] [review]
rm span deps WIP 1

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).
Comment 1 Luke Wagner [:luke] 2012-01-06 16:04:58 PST
*** Bug 715356 has been marked as a duplicate of this bug. ***
Comment 2 Luke Wagner [:luke] 2012-01-06 17:24:28 PST
Created attachment 586626 [details] [diff] [review]
rm span deps

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.
Comment 3 Nicholas Nethercote [:njn] 2012-01-06 18:25:14 PST
Sounds ok to me.
Comment 4 Brian Hackett (:bhackett) 2012-01-07 05:13:07 PST
Also, any extra memory consumed would be gained back by lazy compilation of scripts to bytecode.
Comment 5 Nicholas Nethercote [:njn] 2012-01-07 12:30:16 PST
(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 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-09 13:17:54 PST
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.
Comment 7 Brendan Eich [:brendan] 2012-01-09 13:24:33 PST
Great to see this old code go. It was from simpler days. No JITs, most scripts with jumps much smaller than today, etc.

/be
Comment 8 Luke Wagner [:luke] 2012-01-09 14:01:25 PST
(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
Comment 9 Marco Bonardo [::mak] 2012-01-10 01:41:28 PST
https://hg.mozilla.org/mozilla-central/rev/addfdfd36160

Note You need to log in before you can comment on or make changes to this bug.