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

RESOLVED DUPLICATE of bug 831585

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 831585
5 years ago
3 years ago

People

(Reporter: jimb, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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).
Whiteboard: [js:t]
+∞
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.
(Reporter)

Comment 3

5 years ago
(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?
(Reporter)

Comment 4

5 years ago
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.
cx->iterValue delenda est.

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

Updated

5 years ago
Assignee: general → jimb
(Reporter)

Comment 6

5 years ago
(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.
(Reporter)

Comment 7

5 years ago
Jason and I think this is the right long-term fix for bug 769754 and bug 677957.
Status: NEW → ASSIGNED
(Reporter)

Comment 8

5 years ago
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
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 801721
(Reporter)

Comment 10

5 years ago
Raising priority on this, for reasons explained in comment 9.
Severity: normal → major
(Reporter)

Comment 11

5 years ago
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.
Severity: major → normal
(Reporter)

Comment 12

5 years ago
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.)
Duplicate of this bug: 683694
(Reporter)

Comment 14

5 years ago
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.
(Reporter)

Comment 15

5 years ago
Created 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.
(Reporter)

Comment 16

5 years ago
Created attachment 702876 [details] [diff] [review]
Assert that js_IteratorMore never consumes a pending iterator value.
(Reporter)

Comment 17

5 years ago
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.
(Reporter)

Comment 18

5 years ago
Created attachment 702881 [details] [diff] [review]
Use cx->iterValue for all types of iterators.
(Reporter)

Comment 19

5 years ago
Created attachment 702882 [details] [diff] [review]
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.
(Reporter)

Comment 20

5 years ago
Created attachment 702883 [details] [diff] [review]
Improve comments and variable names in emitting and decompiling for-in loops. No change in behavior.
(Reporter)

Comment 21

5 years ago
Created attachment 702884 [details] [diff] [review]
Replace JSOP_MOREITER and JSOP_ITERNEXT with JSOP_IFMOREITER; remove JSContext::iterValue.
While I am working on the last patch, we could probably already land the other ones.
(Reporter)

Comment 23

5 years ago
(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.
(Reporter)

Comment 24

5 years ago
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.)
(Reporter)

Updated

5 years ago
Summary: MOREITER and ITERNEXT should not save the next iteration value in JSContext → Preparation bug: MOREITER and ITERNEXT should not save the next iteration value in JSContext
(Reporter)

Updated

5 years ago
Attachment #702882 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #702884 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #702875 - Flags: review?(jorendorff)
(Reporter)

Updated

5 years ago
Attachment #702876 - Flags: review?(luke)
(Reporter)

Updated

5 years ago
Attachment #702881 - Flags: review?(luke)
(Reporter)

Updated

5 years ago
Attachment #702883 - Flags: review?(jorendorff)
(Reporter)

Updated

5 years ago
Blocks: 831585
(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

5 years ago
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).

Updated

5 years ago
Attachment #702876 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #702881 - Flags: review?(luke) → review+
(Reporter)

Comment 27

5 years ago
(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

5 years ago
The decompiler isn't exercised by a stock browser, so I am fine with whatever compiles :)
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++
Attachment #702875 - Flags: review?(jorendorff) → review+
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.
Attachment #702883 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 31

5 years ago
(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.
(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...
(Reporter)

Comment 33

5 years ago
Discussion rendered happily pointless. I'll revise the patches in light of the decompiler's removal.
Blocks: 853001
Created attachment 758797 [details]
callstack of assertion

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

up-to-date inbound pull, linux64, debug build
Any progress on this, Jim? This fixes a long-running intermittent orange (bug 853001).
Flags: needinfo?(jimb)
(Reporter)

Comment 36

4 years ago
(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.
Flags: needinfo?(jimb)
(Reporter)

Comment 37

4 years ago
What I should simply say is: "The work isn't done; this needs a SpiderMonkey hacker assigned to it to finish it off."
Naveed, is there anyone on the SM team that could pick this work up?
Flags: needinfo?(nihsanullah)
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 39

3 years ago
(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.
Assignee: jimb → jdemooij
Depends on: 1066197
(Assignee)

Comment 40

3 years ago
I'm fixing this in bug 831585, it turned out to be simpler to do it in one patch.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 831585
You need to log in before you can comment on or make changes to this bug.