[for-of-performance] Handle Async ArrayIterators and a bug fix [v8/v8 : main]
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
->LoadTrue()
.StoreAccumulatorInRegister(done)using `--trace_ignition`, I realized that `done` set to `false` in the second iteration of the loop at this point. The code in line 3323 should set it to `true` at the start of each iteration but it does not. Loading `true` here fixes the bug but I still am not sure why this happens.
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
->LoadTrue()
.StoreAccumulatorInRegister(done)using `--trace_ignition`, I realized that `done` set to `false` in the second iteration of the loop at this point. The code in line 3323 should set it to `true` at the start of each iteration but it does not. Loading `true` here fixes the bug but I still am not sure why this happens.
Ok, let's take this fix for now and look into why 3323 isn't working separately.
Related details
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
->LoadTrue()
.StoreAccumulatorInRegister(done)Leszek Swirskiusing `--trace_ignition`, I realized that `done` set to `false` in the second iteration of the loop at this point. The code in line 3323 should set it to `true` at the start of each iteration but it does not. Loading `true` here fixes the bug but I still am not sure why this happens.
Ok, let's take this fix for now and look into why 3323 isn't working separately.
I figured it out -- it's because of the way the register optimizer works, it expects that `ForOfNext` will _unconditionally_ write into the `done` register, and therefore that the previous store of `True` into the `done` register is not observable.
You can take advantage of this by actually unconditionally writing `True` into `done` at the start of `ForOfNext`, before doing all the callbacks, and then you can move the `LoadTrue().Store(done)` into the `else` branch here.
Related details
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
->LoadTrue()
.StoreAccumulatorInRegister(done)Leszek Swirskiusing `--trace_ignition`, I realized that `done` set to `false` in the second iteration of the loop at this point. The code in line 3323 should set it to `true` at the start of each iteration but it does not. Loading `true` here fixes the bug but I still am not sure why this happens.
Leszek SwirskiOk, let's take this fix for now and look into why 3323 isn't working separately.
I figured it out -- it's because of the way the register optimizer works, it expects that `ForOfNext` will _unconditionally_ write into the `done` register, and therefore that the previous store of `True` into the `done` register is not observable.
You can take advantage of this by actually unconditionally writing `True` into `done` at the start of `ForOfNext`, before doing all the callbacks, and then you can move the `LoadTrue().Store(done)` into the `else` branch here.
That's interesting. I did not know about the register optimizer.
I applied your suggestion but it didn't work. I have uploaded my code so you can see what I have done. I tried loading `True` in bytecode-array-builder.cc and bytecode-graph-builder.cc and none of them worked. I kept both in this CL so you can check them.
(I also tried loading `True` in var_done (TVARIABLE) in `ForOfNextHelper` in code-stub-assembler.cc but that did not look correct to me. We don't have access to the registers there so I did not upload that part in this CL.)
Let me know if I am missing something here, thanks.
Related details
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
->LoadTrue()
.StoreAccumulatorInRegister(done)Leszek Swirskiusing `--trace_ignition`, I realized that `done` set to `false` in the second iteration of the loop at this point. The code in line 3323 should set it to `true` at the start of each iteration but it does not. Loading `true` here fixes the bug but I still am not sure why this happens.
Leszek SwirskiOk, let's take this fix for now and look into why 3323 isn't working separately.
Rezvan Mahdavi HezavehI figured it out -- it's because of the way the register optimizer works, it expects that `ForOfNext` will _unconditionally_ write into the `done` register, and therefore that the previous store of `True` into the `done` register is not observable.
You can take advantage of this by actually unconditionally writing `True` into `done` at the start of `ForOfNext`, before doing all the callbacks, and then you can move the `LoadTrue().Store(done)` into the `else` branch here.
That's interesting. I did not know about the register optimizer.
I applied your suggestion but it didn't work. I have uploaded my code so you can see what I have done. I tried loading `True` in bytecode-array-builder.cc and bytecode-graph-builder.cc and none of them worked. I kept both in this CL so you can check them.
(I also tried loading `True` in var_done (TVARIABLE) in `ForOfNextHelper` in code-stub-assembler.cc but that did not look correct to me. We don't have access to the registers there so I did not upload that part in this CL.)Let me know if I am missing something here, thanks.
What I meant was in the ForOfNext bytecode handler itself, before it calls the ForOfNextHelper. OTOH, this might be tricky with lazy deopts...
Another option you have is to add ForOfNext to the list of bytecodes in the register optimizer which flush the optimizer state. Since you're at the top of a loop, this won't have a huge impact, since loops already flush the registers.
Related details
- Rezvan Mahdavi Hezaveh
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
->LoadTrue()
.StoreAccumulatorInRegister(done)Leszek Swirskiusing `--trace_ignition`, I realized that `done` set to `false` in the second iteration of the loop at this point. The code in line 3323 should set it to `true` at the start of each iteration but it does not. Loading `true` here fixes the bug but I still am not sure why this happens.
Leszek SwirskiOk, let's take this fix for now and look into why 3323 isn't working separately.
Rezvan Mahdavi HezavehI figured it out -- it's because of the way the register optimizer works, it expects that `ForOfNext` will _unconditionally_ write into the `done` register, and therefore that the previous store of `True` into the `done` register is not observable.
You can take advantage of this by actually unconditionally writing `True` into `done` at the start of `ForOfNext`, before doing all the callbacks, and then you can move the `LoadTrue().Store(done)` into the `else` branch here.
Leszek SwirskiThat's interesting. I did not know about the register optimizer.
I applied your suggestion but it didn't work. I have uploaded my code so you can see what I have done. I tried loading `True` in bytecode-array-builder.cc and bytecode-graph-builder.cc and none of them worked. I kept both in this CL so you can check them.
(I also tried loading `True` in var_done (TVARIABLE) in `ForOfNextHelper` in code-stub-assembler.cc but that did not look correct to me. We don't have access to the registers there so I did not upload that part in this CL.)Let me know if I am missing something here, thanks.
What I meant was in the ForOfNext bytecode handler itself, before it calls the ForOfNextHelper. OTOH, this might be tricky with lazy deopts...
Another option you have is to add ForOfNext to the list of bytecodes in the register optimizer which flush the optimizer state. Since you're at the top of a loop, this won't have a huge impact, since loops already flush the registers.
Okay, I applied your suggestion and add the ForOfNext to the list of bytecodes that needs flushing the optimizer state. Thanks for the suggestion.
Related details
- Leszek Swirski
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rezvan Mahdavi Hezaveh (Gerrit)
Rezvan Mahdavi Hezaveh added 1 comment![]( "Open in Gerrit")
Thanks for the review. PTAL, thanks
Related details
- Leszek Swirski
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |