1880670 - app.qonto.com doesn't load correctly (with fixed OptimizeDeadResumePointOperands)

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Nicolas B. Pierron [:nbp — off until 29-09]]

The problems comes from the following function:

We've received a report in https://github.com/webcompat/web-bugs/issues/132940 where the sites fails to load with an error:
Error while processing route: protected.index can't convert symbol to number

The error appears to be happening in this function (in the for (const r in e) t[${ i }.${ r }] = e[r] loop):

            K = Object.prototype.hasOwnProperty;
            function W(e) {
              const t = new G;
              for (const i in e) {
                if (!K.call(e, i)) continue;
                const r = e[i];
                if ('object' == typeof r && r) {
                  const e = W(r);
                  for (const r in e) t[`${ i }.${ r }`] = e[r]
                } else t[i] = r
              }
              return t
            }

The problem is due to a miss understanding between OptimizeDeadResumePointOperands and OptimizeIteratorIndices. The later creates 3 blocks using a copy of the last resume point. However, the copy of the last resume point does not take into account the string concatenation of the interpolation of ${ i }.${ r }, which is used as a key. When we bailout from one of the blocks created by OptimizeIteratorIndices, we do return to a valid resume point whit what was supposed to be valid operands.

However, OptimizeDeadResumePointOperands replaced the operands of these 3 new blocks by the optimized-out magic values. The problem comes from the fact that OptimizeDeadResumePointOperands works on the idea that resume points at the entry of basic block would capture all the operands of all instructions. This is a valid assumption to make given that the graph is produced from the interpreter stack and that stack slots and local are all captured by the resume point operands.

Thus the problem is a miss-understanding on the hidden assumption that blocks' resume point are capturing all the operands of all instructions.

I suggest fixing this issue by doing 3 things:

• Add a flag to annotate blocks where the entry resume point does not capture all the operands of all instructions within the block.
• Explicit this hidden assumption by adding an assertion while verifying the MIRGraph.
• Check for this flag as part of OptimizeDeadResumePointOperands. (as part of Bug 1874456)

This assumption is likely to fail on many places, especially with constants where we add constants as part of the entry block and not where they are supposed to be used. The reason this has worked so far is that constants are generated at-uses in the LIR. So we might have to weaken the assertion for this corner case.

Comment 1

[Iain Ireland [:iain]]

It seems like there are a few places where EliminateDeadResumePointOperands expects invariants to be maintained that the rest of Ion doesn't necessarily know about. I wonder if there's another way to implement EliminateDeadResumePointOperands that is more naturally consistent with the rest of Ion.

nbp, do you have some examples of cases where EliminateDeadResumePointOperands works well, or where it would ideally work well? I understand EliminateTriviallyDeadResumePointOperands: if we will pop a value from the stack immediately after resuming, then there's no point in capturing it in the resume point. What are other cases where we can say with confidence that we'll never need a value after bailing out? I have a vague idea that we might be able to answer this question more robustly by just looking at the bytecode to see if a value is ever used, but I don't have a clear enough sense of the code we're targeting to say for sure.

Comment 2

[Nicolas B. Pierron [:nbp — off until 29-09]]

(In reply to Iain Ireland [:iain] from comment #1)

I understand EliminateTriviallyDeadResumePointOperands: if we will pop a value from the stack immediately after resuming, then there's no point in capturing it in the resume point. What are other cases where we can say with confidence that we'll never need a value after bailing out? I have a vague idea that we might be able to answer this question more robustly by just looking at the bytecode to see if a value is ever used, but I don't have a clear enough sense of the code we're targeting to say for sure.

We should technically be able to claim that we do not need a value after a bailout if it has no removed uses. However the flag does not capture any location, thus we might flag instructions and enforce keeping them allocated beyond the point where it would have been used.

Looking at the interpreter's code might be one way. We would have to capture some locals such as iterators which have to be ended, but apart from that flagging stack operands as they are used by instructions would be one way. Pop instruction being a non-use of a stack slot. However, I am not sure this would cover everything, as one might imagine a local value being copied but not being used.

The problem is that the best way to capture this information would ideally be while visiting the interpreter's code backward, such that we can annotate stack slots and locals as they are being used. I presume we could make it happen in a forward pass, but we would need a mechanism to propagate the collected information back through stack mutations (pop, dup, peek, local)

An alternative might be to do it in 2 phases and 3 states (unknown, used, not-used), the forward phase annotating the resume point operands as we convert the interpreter's bytecode and subsequent phases to do the backward propagation, but we would still require some form of stack mutation recording to apply the backward propagation.

Comment 3

[Iain Ireland [:iain]]

Quick sketch: at some point during WarpBuilder, we add a separate AnnotateUnusedResumeSlots pass. The output of this pass is a bitvector per op (or more efficiently per resumePC) indicating the set of slots that are definitely unused if we resume at that PC. I think we can populate the bitvectors by walking backwards through the bytecode tracking the set of slots that are popped without being used. There are some messy details around eg iterators, but they all seem manageable in principle.

Once we have the set of unused resume slots, we can look at every resume point and replace unused operands with a magic value.

The nice thing about this approach is that it doesn't care what happens during Ion optimization, because it's using the original bytecode as the source of truth. (This makes sense, because the question we're trying to answer is whether a value will be needed after we bail out to the interpreter, which only knows about bytecode.) There shouldn't be any weird interactions with stuff like OptimizeIteratorIndices, and we don't have to worry about identifying and preserving any new invariants.

Thoughts? Would that cover all the intended use cases of the current EliminateDeadResumePointOperands pass?