720316 - Remove indexbase support

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Currently, any script which has more functions, objects, atoms, etc. than UINT16_MAX uses a complicated encoding scheme to store larger indexes.  It's fairly messy complexity, and it makes life trickier for the interpreter, JITs, and so on.  We should just use 32-bit indexes instead for simplicity.

I've started on patches to implement this, converting over minimal sets of opcodes from 16-bit indexes + indexbase to 32-bit indexes.  I don't have it all done yet, but I've got a few patches for some portions of this work to post.  Remaining bits should follow over the next couple days.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Introduce GET_UINT8/SET_UINT8 helpers

Just noticed in passing, seems better to be clean about this...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Remove JOF_SLOTATOM

It's unused.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Make regexp indexes 32 bits

This is probably the easiest of the indexes to make 32 bits, because nothing else (no other opcodes, methods, etc.) implicitly or explicitly depends on this.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Make object and function indexes 32 bits

Let's spread the review love around a bit.

Still to go are double indexes and atom indexes.  There might be more splitup to do here, not clear -- doubles used to be encoded as atoms, so there may be some intertwinedness that'll require fixing both at once.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 590834 [details] [diff] [review]
Introduce GET_UINT8/SET_UINT8 helpers

Review of attachment 590834 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinterp.cpp
@@ +2125,5 @@
>  BEGIN_CASE(JSOP_PICK)
>  {
> +    uint8_t i = GET_UINT8(regs.pc);
> +    JS_ASSERT(regs.sp - (i + 1) >= regs.fp()->base());
> +    Value lval = regs.sp[-int8_t(i + 1)];

EmitDestructingOpsHelper emits uint8's up to jsbytecode(-1) (should be changed to UINT8_MAX) which I think breaks this.  Thus even uint8_t would be the wrong type here.  I'd prefer 'i' to be 'unsigned' and then this expression to be 'regs.sp[-int(i + 1)]'.

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/271838a8bc5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2a7edff3c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d4887aae8d

More work to be done here, don't close yet...

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/271838a8bc5e
https://hg.mozilla.org/mozilla-central/rev/6a2a7edff3c5
https://hg.mozilla.org/mozilla-central/rev/f8d4887aae8d

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 590838 [details] [diff] [review]
Make object and function indexes 32 bits

All right. Finish it!

Comment 9

[Jeff Walden [:Waldo]]

dvander, the object/function index patch here breaks these tests:

http://hg.mozilla.org/mozilla-central/file/default/js/src/jit-test/tests/jaeger/bug563000/trap-from-add-inline.js
http://hg.mozilla.org/mozilla-central/file/default/js/src/jit-test/tests/jaeger/bug563000/trap-from-add-ool.js

Both claim the number being trapped is the JSOP_STOP in main.  Right now, tho, they're hitting the JSOP_POP right before JSOP_STOP in main.  Should I repoint them at JSOP_STOP when I land that patch?

(Also, good grief is trap fragile.  We should have a function which takes the index, the bytecode at that location, and perhaps the surrounding two bytecodes to verify that bytecode changes don't end up silently making trap-using tests not test what they were intended to test.)

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Jeff Walden (remove +bmo to email) from comment #9)
> Both claim the number being trapped is the JSOP_STOP in main.  Right now,
> tho, they're hitting the JSOP_POP right before JSOP_STOP in main.  Should I
> repoint them at JSOP_STOP when I land that patch?
> 
> (Also, good grief is trap fragile.  We should have a function which takes
> the index, the bytecode at that location, and perhaps the surrounding two
> bytecodes to verify that bytecode changes don't end up silently making
> trap-using tests not test what they were intended to test.)

Yeah, they are very fragile. Ideally they would specify a line number or something. I'm not sure what opcode they're supposed to be pointing at, usually I just +/- 1 or so until they work again, but recently I had to fix these as well and I think I landed on the JSOP_POP.

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08f4fc94e27b

Still don't close this, more to come...

Comment 12

[Matt Brubeck (:mbrubeck)]

Sorry, had to back this out on inbound because it (or something it landed with) had some jsreftest issues:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a22cb315b248

Comment 13

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/551dcf40a209

with the Windows-specific bustage fix attempted yesterday (successfully, but other stuff was broken from different commits).

Still more to come.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/551dcf40a209

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Near-final patch, one issue remains

This passes every test but regress-634472.js.  Reduced and with some jit-args futzing, it comes to this:

[jwalden@wheres-wally src]$ cat /tmp/test.js 
var x = [];
var cases = [{q:1}, {q:2}];

for (let i = 0; i < 2; i++)
  cases[i].z;
[jwalden@wheres-wally src]$ dbg/js -m -a -f /tmp/test.js 
/tmp/test.js:5: TypeError: cases[i] is undefined

Here's a variant that also has a weird failure mode:

[jwalden@wheres-wally src]$ cat /tmp/experiment.js 
var x = [];
var cases = [{z:1}, {z:2}];

Array.prototype[1] = { z: "foo" };

var q;
for (let i = 0; i < 2; i++)
  q = cases[i].z;

print(q);
[jwalden@wheres-wally src]$ dbg/js -m -a -f /tmp/experiment.js 
1
[jwalden@wheres-wally src]$ ~/moz/mfbt/js/src/dbg/js -m -a -f /tmp/experiment.js # the correct result
2

I'm not getting very far figuring out what's wrong with this.  :-\  Stepping a little in a debugger, it appears that js::mjit::stubs::GetElement is being called with the wrong object -- the object |x| (the length-zero array), not the object |cases|.  I'm at a loss to say how this is happening right now.

Anyway, that's the status right now.  I'm still trying to figure out what's wrong here, and why my changes would have triggered this in the first place.

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Remove indexbase from all remaining ops

Luke is my hero forever.

Comment 17

[Jason Orendorff [:jorendorff]]

Wait. What does this patch contain? Does it include all four patches? What part of it hasn't been reviewed already?

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 595110 [details] [diff] [review]
Remove indexbase from all remaining ops

It's separate from the other patches (all of which landed).  Those patches could be done separately, so in the interests of smaller patches and easier debugging I did them separately.  Unfortunately JOF_ATOM for the rest is quite monolithic, so everything in this patch has to be done at once.  (You could extract the JSOP_DOUBLE bits, but those are small enough that it wasn't really worth extracting that out into another patch, after the fact.)

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 595110 [details] [diff] [review]
Remove indexbase from all remaining ops

>--- a/js/src/jsinfer.cpp
>+++ b/js/src/jsinfer.cpp
>@@ -2035,21 +2035,21 @@ TypeCompartment::newAllocationSiteTypeOb
> static inline jsid
> GetAtomId(JSContext *cx, JSScript *script, const jsbytecode *pc, unsigned offset)
> {
>-    unsigned index = js_GetIndexFromBytecode(script, (jsbytecode*) pc, offset);
>+    unsigned index = js_GetIndexFromBytecode((jsbytecode*) pc, offset);
>     return MakeTypeId(cx, ATOM_TO_JSID(script->getAtom(index)));
> }
> 
> static inline JSObject *
> GetScriptObject(JSContext *cx, JSScript *script, const jsbytecode *pc, unsigned offset)
> {
>-    unsigned index = js_GetIndexFromBytecode(script, (jsbytecode*) pc, offset);
>+    unsigned index = js_GetIndexFromBytecode((jsbytecode*) pc, offset);
>     return script->getObject(index);
> }
> 
> static inline const Value &
> GetScriptConst(JSContext *cx, JSScript *script, const jsbytecode *pc)
> {
>-    unsigned index = js_GetIndexFromBytecode(script, (jsbytecode*) pc, 0);
>+    unsigned index = js_GetIndexFromBytecode((jsbytecode*) pc, 0);
>     return script->getConst(index);
> }
> 

Want to make those explicit const_casts while you're here?

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Rebased

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 597160 [details] [diff] [review]
Rebased

You might want to bump JSXDR_BYTECODE_VERSION after this.

In frontend/BytecodeEmitter.cpp, frontend::EmitTree:
>       case PNK_XMLATTR:
>       case PNK_XMLSPACE:
>       case PNK_XMLTEXT:
>-      case PNK_XMLCDATA:
>-      case PNK_XMLCOMMENT:
>-        JS_ASSERT(!bce->inStrictMode());
>         /* FALL THROUGH */
> #endif
>       case PNK_STRING:
>         ok = EmitAtomOp(cx, pn, pn->getOp(), bce);
>         break;
> 
>+#if JS_HAS_XML_SUPPORT
>+      case PNK_XMLCDATA:
>+      case PNK_XMLCOMMENT:
>+        JS_ASSERT(!bce->inStrictMode());
>+        ok = EmitAtomOp(cx, pn, pn->getOp(), bce);
>+        break;
>+#endif

What changed here? Wasn't it OK the way it was?  Didn't you basically
remove an assertion from the PNK_XML{ATTR,SPACE,TEXT} cases?  I'm all
for cloning the EmitAtomOp call instead of FALL THROUGH, if you want.

In jsanalyze.cpp, ScriptAnalysis::checkAliasedName:
>+        atom = script->getAtom(GET_UINT32_INDEX(pc));

A convenience method script->getAtomAt(pc) would be nice.

In jsinterp.cpp, js::Interpret:
> # define END_CASE_LEN4      len = 4; goto advance_pc;
> # define END_CASE_LEN5      len = 5; goto advance_pc;
>+# define END_CASE_LEN6      len = 6; goto advance_pc;
> # define END_CASE_LEN7      len = 7; goto advance_pc;

lol

In jsobj.cpp, Detecting:
>-            /*
>-             * At this point, anything but an extended atom index prefix means
>-             * we're not detecting.
>-             */
>-            if (!(js_CodeSpec[op].format & JOF_INDEXBASE))
>-                return JS_FALSE;
>-            break;
>+            return JS_FALSE;
>         }
>     }
> }

Yay! You can get rid of the loop now. While you're doing that, might as
well switch to C++ true and false.

In jsopcode.h:
>  * A slower version of GET_ATOM when the caller does not want to maintain
>  * the index segment register itself.

This comment already didn't make sense, but now it doesn't make even
more sense. Fix it?

In GetDecomposeLength:
>     /*
>-     * The last byte of a DECOMPOSE op stores the decomposed length. This can
>-     * vary across different instances of an opcode due to INDEXBASE ops.
>+     * The last byte of a DECOMPOSE op stores the decomposed length.  This is a
>+     * constant: perhaps we should just hardcode values instead?
>      */

bhackett, I think you wrote this comment originally. Do you want a
follow-up bug for this?

Comment 22

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e955f78de9

And that finishes off this bug for good now.  \o/  Merge-monkeys may close this bug after this last merge to m-c.

(In reply to Jason Orendorff [:jorendorff] from comment #21)
> You might want to bump JSXDR_BYTECODE_VERSION after this.

Done.  I'm pretty sure I'd been bumping it in every other patch, and either I forgot here or someone else bumped it and rebasing didn't expose it as a conflict at some point.  I'm betting on the latter.  ;-)

> What changed here? Wasn't it OK the way it was?  Didn't you basically
> remove an assertion from the PNK_XML{ATTR,SPACE,TEXT} cases?  I'm all
> for cloning the EmitAtomOp call instead of FALL THROUGH, if you want.

Hmm.  I remember making a change here (or starting to make one) out of necessity, but I don't remember what that was, and now it's not obvious why I was making it.  Reverted this bit, don't feel strongly about removing fallthrough here.

> In jsanalyze.cpp, ScriptAnalysis::checkAliasedName:
> >+        atom = script->getAtom(GET_UINT32_INDEX(pc));
> 
> A convenience method script->getAtomAt(pc) would be nice.

Luke and I discussed this briefly and agreed that we really should make bytecode streams some sort of abstract, class-y view of the underlying bytes -- no jsbytecode* that gets abused horribly in every use.  I guess that doesn't really conflict with this, but I kind of think it'd be nice to solve all these convenience methods in one coherent whole rather than pick off pieces at a time possibly inconsistently.  I'll file a bug on making a nicer bytecode sequence interface that can sweep this up.

> In jsinterp.cpp, js::Interpret:
> > # define END_CASE_LEN4      len = 4; goto advance_pc;
> > # define END_CASE_LEN5      len = 5; goto advance_pc;
> >+# define END_CASE_LEN6      len = 6; goto advance_pc;
> > # define END_CASE_LEN7      len = 7; goto advance_pc;
> 
> lol

...yeah.  I broke Windows because of this in one of the previous patches here, I think.  :-\  We probably should just remove the threaded-interpreter code rather than have to treat Windows specially like this.

> In jsopcode.h:
> >  * A slower version of GET_ATOM when the caller does not want to maintain
> >  * the index segment register itself.
> 
> This comment already didn't make sense, but now it doesn't make even
> more sense. Fix it?

I removed it entirely -- I can't think of a way to fix it that doesn't just make it mostly a nullity.

> In GetDecomposeLength:
> >     /*
> >-     * The last byte of a DECOMPOSE op stores the decomposed length. This can
> >-     * vary across different instances of an opcode due to INDEXBASE ops.
> >+     * The last byte of a DECOMPOSE op stores the decomposed length.  This is a
> >+     * constant: perhaps we should just hardcode values instead?
> >      */
> 
> bhackett, I think you wrote this comment originally. Do you want a
> follow-up bug for this?

CCing in case he doesn't watch the component (I don't know one way or the other)...

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/f4e955f78de9

Comment 24

[Brian Hackett [Laid off!]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #22)
> > In GetDecomposeLength:
> > >     /*
> > >-     * The last byte of a DECOMPOSE op stores the decomposed length. This can
> > >-     * vary across different instances of an opcode due to INDEXBASE ops.
> > >+     * The last byte of a DECOMPOSE op stores the decomposed length.  This is a
> > >+     * constant: perhaps we should just hardcode values instead?
> > >      */
> > 
> > bhackett, I think you wrote this comment originally. Do you want a
> > follow-up bug for this?
> 
> CCing in case he doesn't watch the component (I don't know one way or the
> other)...

Sure, it would be good to simplify the decompose stuff.  Until I hit the INDEXBASE problem, earlier patches in that bug hardcoded things.  (I watch NEW bugs in JS, but not other activity).

Comment 25

[Jeff Walden [:Waldo]]

There's a claim that something in the overall push that included this last change caused a heap-size regression:

https://bugzilla.mozilla.org/show_bug.cgi?id=728411#c18

This seems the most likely candidate, but even still I'd be a bit surprised about this.  A 2% increase seems way higher than I'd expect for any change from this.

As far as diagnosing the actual cause...um.  Maybe I have time tomorrow to back everything out and land stuff piecemeal, or something.

Comment 26

[Luke Wagner [:luke]]

Before doing anything drastic, why don't you run membuster (http://gregor-wagner.com/tmp/mem) and compare overall explicit change as well as script bytes.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 28

[Jan de Mooij [:jandem]]

This no longer applies AFAICT.