Closed Bug 1344753 Opened 3 years ago Closed 3 years ago

Update ControlFlowGenerator::processWhileOrForInLoop to follow bug 1342553 ?

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file)

Just found I overlooked one place that touches the stack depth for for-of, that was changed from 2 to 3 in bug 1342553.
Honestly I'm not sure if this is correct fix, nor how this can cause an issue.

Bug 1342553 changes the stack layout of for-of from
  ... ITER RESULT ...
to
  ... ITER RESULT VALUE ...

and stackPhiCount in ControlFlowGenerator::processWhileOrForInLoop sounds related to that number.
Attachment #8844003 - Flags: review?(jdemooij)
Comment on attachment 8844003 [details] [diff] [review]
Update for-of stack depth in ControlFlowGenerator::processWhileOrForInLoop.

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

We avoid creating phis for these stackPhiCount values on top of the stack: http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/js/src/jit/MIRGraph.cpp#594-601 Does that still make sense for these 3 values?

We should probably backport this to 54 just in case.
Attachment #8844003 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #3)
> We avoid creating phis for these stackPhiCount values on top of the stack:
> http://searchfox.org/mozilla-central/rev/
> 1bc7720a434af3c13b68b8d69f92078cae8890c4/js/src/jit/MIRGraph.cpp#594-601

I'm not sure why we can skip creating phis there.
is it because those stack values aren't changed for between iterations, or some other reason?

at least "RESULT" differs between iterations, so I guess I'm wrong.  and newly added "VALUE" also changes along with "RESULT".
perhaps I'm completely misunderstanding what phi is :P
Flags: needinfo?(jdemooij)
Hm good point, I'll look into that now.
(In reply to Jan de Mooij [:jandem] from comment #3)
> We avoid creating phis for these stackPhiCount values on top of the stack:
> http://searchfox.org/mozilla-central/rev/
> 1bc7720a434af3c13b68b8d69f92078cae8890c4/js/src/jit/MIRGraph.cpp#594-601
> Does that still make sense for these 3 values?

Actually we *do* create phis for the stackPhiCount values on top of the stack, but we *avoid* creating phis for the stack values below them. wingo added this a few years ago, it came up with object/array comprehensions.

Given the status of comprehensions (not standardized), we could block them from Ion and remove this complexity, but for now your patch is fine.
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Jan de Mooij [:jandem] from comment #3)
> > We avoid creating phis for these stackPhiCount values on top of the stack:
> > http://searchfox.org/mozilla-central/rev/
> > 1bc7720a434af3c13b68b8d69f92078cae8890c4/js/src/jit/MIRGraph.cpp#594-601
> > Does that still make sense for these 3 values?
> 
> Actually we *do* create phis for the stackPhiCount values on top of the
> stack, but we *avoid* creating phis for the stack values below them. wingo
> added this a few years ago, it came up with object/array comprehensions.
> 
> Given the status of comprehensions (not standardized), we could block them
> from Ion and remove this complexity, but for now your patch is fine.

ah, makes sense :)
I'll land the current patch and backport it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/047b8fb1c6a353d12fe48008d64b81946ba757cb
Bug 1344753 - Update for-of stack depth in ControlFlowGenerator::processWhileOrForInLoop. r=jandem
https://hg.mozilla.org/mozilla-central/rev/047b8fb1c6a3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8844003 [details] [diff] [review]
Update for-of stack depth in ControlFlowGenerator::processWhileOrForInLoop.

Approval Request Comment
> [Feature/Bug causing the regression]
Bug 1342553

> [User impact if declined]
A bit more memory consumption while JIT compilation

> [Is this code covered by automated tests?]
Not by specific one, but the path is covered by many tests.

> [Has the fix been verified in Nightly?]
Yes

> [Needs manual test from QE? If yes, steps to reproduce]
No

> [List of other uplifts needed for the feature/fix]
None

> [Is the change risky?]
No.

> [Why is the change risky/not risky?]
just reduces the unnecessary memory allocation

> [String changes made/needed]
None
Attachment #8844003 - Flags: approval-mozilla-aurora?
Comment on attachment 8844003 [details] [diff] [review]
Update for-of stack depth in ControlFlowGenerator::processWhileOrForInLoop.

Avoid memory consumption while JIT compilation. Aurora54+.
Attachment #8844003 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.