de-OptimizeSpanDeps

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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).
(Assignee)

Updated

6 years ago
Duplicate of this bug: 715356
(Assignee)

Comment 2

6 years ago
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.
Assignee: general → luke
Attachment #586562 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #586626 - Flags: review?(jwalden+bmo)
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.
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
(Assignee)

Comment 8

6 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.