777596 - Preparation bug: MOREITER and ITERNEXT should not save the next iteration value in JSContext

Status: RESOLVED DUPLICATE of bug 831585

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

The MOREITER bytecode determines whether an iteration should continue, and ITERNEXT retrieves the iteration's next value. Depending on what's being iterated over, MOREITER may, as a side effect, actually produce the value for the next iteration.

As presently implemented, MOREITER saves the value it produces, if any, in JSContext::iterValue, and ITERNEXT tries to retrieve it from there. Bugs 772770 and bug 769754 demonstrate why this is a poor design: through various mechanisms (Debugger.Frame onStep handlers; proxies on the scope chain; and who knows what else), arbitrary JS code can run between those two bytecodes; and if that intervening code does any iteration itself, it will trash the interrupted code's iteration value that ITERNEXT was expecting.

Instead, MOREITER should produce *two* stack values: the next value, or MagicValue(JS_NO_ITER_VALUE) if none is available; and the boolean value indicating whether the iteration should continue, as it does now. ITERNEXT should then consult that stack slot when appropriate. By storing the pending value in the stack frame instead of JSContext, there will be no need for explicit code to save and restore it (like PreserveIterValue).

Comment 1

[Jason Orendorff [:jorendorff]]

+∞

Comment 2

[Brian Hackett [Laid off!]]

Are there cases when the JS_NO_ITER_VALUE is produced and iteration should continue, or when a different value is produced and iteration should stop?  It'd be nice for the JITs (IM especially) if the opcode has only a single pushed value.

Comment 3

[Jim Blandy :jimb]

(In reply to Brian Hackett (:bhackett) from comment #2)
> Are there cases when the JS_NO_ITER_VALUE is produced and iteration should
> continue, or when a different value is produced and iteration should stop? 
> It'd be nice for the JITs (IM especially) if the opcode has only a single
> pushed value.

I don't see why JSOP_MOREITER couldn't push JS_NO_ITER_VALUE to indicate that iteration should terminate. We'd need JS_IFNOITER, or we'd need to fuse the conditional jump with the push. Which option would be better for IM and JM?

Comment 4

[Jim Blandy :jimb]

It worries me that js_IteratorMore checks cx->iterValue: "We might still have a pending value." That means that iterValue is an *input* to js_IteratorMore, not just an output. I've put an assertion there, to see in which cases this arises.

Comment 5

[Jason Orendorff [:jorendorff]]

cx->iterValue delenda est.

What if we just had one fat opcode at the bottom of the loop, say, JSOP_NEXT?

Comment 6

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #4)
> It worries me that js_IteratorMore checks cx->iterValue: "We might still
> have a pending value." That means that iterValue is an *input* to
> js_IteratorMore, not just an output. I've put an assertion there, to see in
> which cases this arises.

No jit-test or js/src/tests test triggers this assertion.

Comment 7

[Jim Blandy :jimb]

Jason and I think this is the right long-term fix for bug 769754 and bug 677957.

Comment 8

[Jim Blandy :jimb]

Here's a try push for a patch that adds an assertion that js_IteratorMore never consumes a pending iterator value. The tests under js/src suggest that this is true; and if it is, it will simplify a fix.

https://tbpl.mozilla.org/?tree=Try&rev=80a610a8db07

Comment 9

[Jim Blandy :jimb]

Should we increase the priority of this bug?

This assertion could trip any time we pop up a slow script dialog. And it seems possible that slow script dialogs might be more likely to pop up when we're executing for-in loops.

Comment 10

[Jim Blandy :jimb]

Raising priority on this, for reasons explained in comment 9.

Comment 11

[Jim Blandy :jimb]

The approach we're considering is this:

First, make NativeIterator key iterators produce their values at MOREITER time, like all other iterators. At present, JSOP_ITERNEXT would be little more than "return cx->iterValue" except that it checks for NativeIterators and produces their value at that point, instead of in JSOP_MOREITER.

Then, simultaneously:

- Change JSOP_MOREITER to push MagicValue(JS_NO_ITER_VALUE) if iteration should stop, or the next iteration value if it should continue.

- Add a new opcode, JSOP_IFMOREITER, that branches if the top of the stack is not MagicValue(JS_NO_ITER_VALUE), leaving the value on the stack, and pops the value otherwise.

- Stop emitting JSOP_ITERNEXT altogether; the value is already on the top of the stack at the next iteration.

Comment 12

[Jim Blandy :jimb]

Currently, a for-in loop produces bytecode like this:

        ... get iterable object on top of stack
00003:  iter 1
00005:  goto 42 (+37)
00010:  loophead
00011:  iternext
        ... body of loop
00042:  loopentry
00043:  moreiter
00044:  ifne 10 (-34)
00049:  enditer
        ... 

In the comment above, I'm proposing we generate code like this:

        ... get iterable object on top of stack
00003:  iter 1
00005:  goto 42 (+37)
00010:  loophead
        ... body of loop
00042:  loopentry
00043:  moreiter
00044:  ifmoreiter 10 (-34)
00049:  enditer
        ...

(The truth is, I don't really know what enditer does, but it's probably not too important.)

Comment 14

[Jim Blandy :jimb]

Status update: I've got a patch for the interpreter only. It passes all tests, and handles the decompiler (which still exists). I'm trying to expand it to cover jsanalyze.cpp's SSA analysis and TI, and then JM, and then IM. I think it should be okay.

Comment 15

[Jim Blandy :jimb]

Attachment: Expose the decompiler via TestingFunctions. If we're not going to delete it, we have to be able to test it.

Comment 16

[Jim Blandy :jimb]

Attachment: Assert that js_IteratorMore never consumes a pending iterator value.

Comment 17

[Jim Blandy :jimb]

Regarding that attachment:

The comment in the original code says we might have a pending value here,
but no test in js/src/jit-test or js/src/tests produces such a state of
affairs. Fixing this bug will be simpler if we can assume that MOREITER
only writes to JSContext::iterValue, and never reads from it.

Comment 18

[Jim Blandy :jimb]

Attachment: Use cx->iterValue for all types of iterators.

Comment 19

[Jim Blandy :jimb]

Attachment: Remove native iterator fast paths.

I haven't measured the impact of this patch, but it should make it simpler
to convert the code to a fused JSOP_IFMOREITER bytecode. This optimization
should be easy to re-introduce if it's valuable once the conversion is
done.

Comment 20

[Jim Blandy :jimb]

Attachment: Improve comments and variable names in emitting and decompiling for-in loops. No change in behavior.

Comment 21

[Jim Blandy :jimb]

Attachment: Replace JSOP_MOREITER and JSOP_ITERNEXT with JSOP_IFMOREITER; remove JSContext::iterValue.

Comment 22

[Tom S. (please needinfo tschuster)]

While I am working on the last patch, we could probably already land the other ones.

Comment 23

[Jim Blandy :jimb]

(In reply to Tom Schuster [:evilpie] from comment #22)
> While I am working on the last patch, we could probably already land the
> other ones.

It's a lot easier for the build people to keep track of things if bugs with many patches land either none or all of their patches at once. Let's drop the last patch, rename this bug to "preparation", get review on the patches, and then let you continue your work in another bug.

Comment 24

[Jim Blandy :jimb]

Actually, we should probably not land "remove native iterator fast paths" either, until we've established that it has no perf effect. (I don't have time to pursue that right now.)

Comment 25

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

(In reply to Jim Blandy :jimb from comment #23)
> (In reply to Tom Schuster [:evilpie] from comment #22)
> > While I am working on the last patch, we could probably already land the
> > other ones.
> 
> It's a lot easier for the build people to keep track of things if bugs with
> many patches land either none or all of their patches at once. Let's drop
> the last patch, rename this bug to "preparation", get review on the patches,
> and then let you continue your work in another bug.

Not sure who the build people are, but with [leave open] in the whiteboard and checkin+ on the landed patches, nobody will be confused.

Comment 26

[Luke Wagner [:luke]]

Can we please not expose the decompiler for any testing?  It is coming time to remove that code (as Benjamin's work is close to (or already made) release so it doesn't seem like we should waste anyone's time on it.  The only use is the jit inspector and for that we should be able to do something else (like just print the information next to the opcode).

Comment 27

[Jim Blandy :jimb]

(In reply to Luke Wagner [:luke] from comment #26)
> Can we please not expose the decompiler for any testing?  It is coming time
> to remove that code (as Benjamin's work is close to (or already made)
> release so it doesn't seem like we should waste anyone's time on it.  The
> only use is the jit inspector and for that we should be able to do something
> else (like just print the information next to the opcode).

If you're okay with my decompiler patch going in without test coverage, that's totally fine with me.

If not, the decompiler needs some kind of change once JSOP_MOREITER is deleted. The simplest response I can imagine is adding JS_NOT_REACHED and just letting the decompiler crash, but it smacks of perversity to be sticking that in code that one knows is reachable. The most thorough response I can imagine would be making this bug block on deleting the decompiler, but that also seems like a bogus dependency, unless someone expects it done sooner than this bug.

Comment 28

[Luke Wagner [:luke]]

The decompiler isn't exercised by a stock browser, so I am fine with whatever compiles :)

Comment 29

[Jason Orendorff [:jorendorff]]

Comment on attachment 702875 [details] [diff] [review]
Expose the decompiler via TestingFunctions. If we're not going to delete it, we have to be able to test it.

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

decompiler_tco++

Comment 30

[Jason Orendorff [:jorendorff]]

Comment on attachment 702883 [details] [diff] [review]
Improve comments and variable names in emitting and decompiling for-in loops. No change in behavior.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4510,5 @@
>      Rooted<StaticBlockObject*> blockObj(cx, letDecl ? &pn1->pn_objbox->object->asStaticBlock() : NULL);
>      uint32_t blockObjCount = blockObj ? blockObj->slotCount() : 0;
>  
> +    /*
> +     * The bytecode for 'for (LHS in RHS) BODY' looks like the below.

"looks like the below" is awkward; I'd just delete this sentence.

@@ +4512,5 @@
>  
> +    /*
> +     * The bytecode for 'for (LHS in RHS) BODY' looks like the below.
> +     *
> +     * Note that 'for (let LHS in RHS) BODY' requires some careful ordering: the

And I guess I'd remove "Note that" from this one.

Comment 31

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #27)
> If you're okay with my decompiler patch going in without test coverage,
> that's totally fine with me.

You know, I think I want to push back on this a little bit.

I don't really understand why it's bad to expose something for testing. Only the tests specifically concerned with it will ever use that; they'll be so easy to ditch. I don't really see the slippery slope. And in my new religion, I don't believe code does anything in particular until it's tested.

Comment 32

[Jason Orendorff [:jorendorff]]

(In reply to Luke Wagner [:luke] from comment #26)
> Can we please not expose the decompiler for any testing?  It is coming time
> to remove that code (as Benjamin's work is close to (or already made)
> release so it doesn't seem like we should waste anyone's time on it.

It would be great to delete the decompiler. I hope we do it next week.

In the meantime, a little test coverage for features someone is using, as much as I wish they wouldn't, is not so bad. Let's get this in and move on...

Comment 33

[Jim Blandy :jimb]

Discussion rendered happily pointless. I'll revise the patches in light of the decompiler's removal.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Attachment: callstack of assertion

I ran into this on a debug run (annoying).

up-to-date inbound pull, linux64, debug build

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Any progress on this, Jim? This fixes a long-running intermittent orange (bug 853001).

Comment 36

[Jim Blandy :jimb]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> Any progress on this, Jim? This fixes a long-running intermittent orange
> (bug 853001).

You don't mean "this fixes", you mean "this would fix", right? This bug got turned into a prep patch bug, and the real work is in bug 831585. There, we never finished IonMonkey support (unless there's been a lot of activity in some unrelated bug). So the fix isn't written yet.

Finishing it is a non-trivial project, although perhaps an IonMonkey hacker could bash it out quickly. Tom had volunteered to work on this; I think he gave up and set it aside. I'm no longer on the SpiderMonkey team, so I probably shouldn't be working on it at all.

Comment 37

[Jim Blandy :jimb]

What I should simply say is: "The work isn't done; this needs a SpiderMonkey hacker assigned to it to finish it off."

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Naveed, is there anyone on the SM team that could pick this work up?

Comment 39

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #35)
> Any progress on this, Jim? This fixes a long-running intermittent orange
> (bug 853001).

The orange seems to be gone after changes to the test, but the bug is probably still there. I'm interested in optimizing for-in so I might as well fix this first. Stealing with permission.

Comment 40

[Jan de Mooij [:jandem]]

I'm fixing this in bug 831585, it turned out to be simpler to do it in one patch.