Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487483004:
Only treat possible eval calls going through 'with' as special. (Closed)
DescriptionOnly treat possible eval calls going through 'with' as special.
This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL
or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime
call to find the callee and possibly update the receiver with the with-object.
This means that eval calls out of 'with' blocks can now just do a normal
LdaLookupGlobalSlot operation, which can check the context chain for eval
extentions and fast-path the lookup if none exist.
BUG=661556
Committed: https://crrev.com/be9b820c4405e3feb1bcc60f5bf257c574da76c9
Cr-Commit-Position: refs/heads/master@{#40965}
Patch Set 1 #Patch Set 2 : Fix AstGraphBuilder #
Total comments: 3
Patch Set 3 : Remove POSSIBLY_EVAL_CALL #Patch Set 4 : Rebase #Patch Set 5 : Rebase expectations #Patch Set 6 : Fix ast-graph-builder #
Total comments: 2
Patch Set 7 : Address comments #Patch Set 8 : Rebase #Messages
Total messages: 61 (44 generated)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15830) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15869) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28086)
Patchset #1 (id:1) has been deleted
rmcilroy@chromium.org changed reviewers: + verwaest@chromium.org
This builds on Toon's CL in https://codereview.chromium.org/2480253006 to do the same treatment for eval. With this change we get back on-par with the pre-ignition-on-tf performance for date-format-tofte (at least on my local machine. PTAL, thanks.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rmcilroy@chromium.org changed reviewers: + mstarzinger@chromium.org
This builds on Toon's CL in https://codereview.chromium.org/2480253006 to do the same treatment for eval. With this change we get back on-par with the pre-ignition-on-tf performance for date-format-tofte (at least on my local machine. PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15870) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12127) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
On 2016/11/08 17:26:04, rmcilroy wrote: > This builds on Toon's CL in https://codereview.chromium.org/2480253006 to do the > same treatment for eval. With this change we get back on-par with the > pre-ignition-on-tf performance for date-format-tofte (at least on my local > machine. > > PTAL, thanks. Damn, looks like I'll need to tweak this a bit to get cctests working (I thought all the relevant tests would be in mjsunit, and they all passed). I'll ping again when I have a fix, don't review until then.
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK fixed things (I had made a change in ast-graph-builder without the same change in FCG, so things broke). Michi: Do you know that the FunctionTester is testing ast-graph-builder. Is this as-expected or should it be testing bytecode-graph-builder now? Either way, PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I actually think it would be cleaner to get rid of POSSIBLY_EVAL_CALL altogether, and check that separately. POSSIBLY_EVAL_CALL should always do what OTHER_CALL does, and POSSIBLY_EVAL_CALL_THROUGH_WITH should always do what WITH_CALL does. The only diff is that possibly_eval has other behavior after the switch. https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... src/full-codegen/full-codegen.cc:1672: case Call::POSSIBLY_EVAL_THROUGH_WITH_CALL: Shouldn't POSSIBLY_EVAL_CALL be moved to OTHER_CALL now? Here and the ast-graph-builder?
> I actually think it would be cleaner to get rid of POSSIBLY_EVAL_CALL > altogether, and check that separately. POSSIBLY_EVAL_CALL should always do what > OTHER_CALL does, and POSSIBLY_EVAL_CALL_THROUGH_WITH should always do what > WITH_CALL does. The only diff is that possibly_eval has other behavior after the > switch. I could do this but would prefer to do it in a followup CL, WDYT? https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... src/full-codegen/full-codegen.cc:1672: case Call::POSSIBLY_EVAL_THROUGH_WITH_CALL: On 2016/11/09 14:13:08, Toon Verwaest wrote: > Shouldn't POSSIBLY_EVAL_CALL be moved to OTHER_CALL now? Here and the > ast-graph-builder? Nope because we also need to resolve the eval function for POSSIBLY_EVAL_CALL which we don't do with OTHER_CALL (there is no possibly_eval branch below like there is in ast-graph-builder / bytecode-graph-builder). Given that we never actually go through FCG for functions with eval any longer (since they will be optimized by Turbofan, so always go through Ignition after the change which feeds all TF code via Ignition) I don't think it's worth doing any work here - we will eventually rip support for eval out of FCG once we are sure the TF via Ignition change is going to stick.
Ok, I'm fine with landing it this way given that FCG w/ POSSIBLY_EVAL_CALL is still different. https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... src/full-codegen/full-codegen.cc:1672: case Call::POSSIBLY_EVAL_THROUGH_WITH_CALL: On 2016/11/09 17:02:29, rmcilroy wrote: > On 2016/11/09 14:13:08, Toon Verwaest wrote: > > Shouldn't POSSIBLY_EVAL_CALL be moved to OTHER_CALL now? Here and the > > ast-graph-builder? > > Nope because we also need to resolve the eval function for POSSIBLY_EVAL_CALL > which we don't do with OTHER_CALL (there is no possibly_eval branch below like > there is in ast-graph-builder / bytecode-graph-builder). > > Given that we never actually go through FCG for functions with eval any longer > (since they will be optimized by Turbofan, so always go through Ignition after > the change which feeds all TF code via Ignition) I don't think it's worth doing > any work here - we will eventually rip support for eval out of FCG once we are > sure the TF via Ignition change is going to stick. I see. But it is still relevant in the ast-graph-builder though right?
> https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... > src/full-codegen/full-codegen.cc:1672: case > Call::POSSIBLY_EVAL_THROUGH_WITH_CALL: > On 2016/11/09 17:02:29, rmcilroy wrote: > > On 2016/11/09 14:13:08, Toon Verwaest wrote: > > > Shouldn't POSSIBLY_EVAL_CALL be moved to OTHER_CALL now? Here and the > > > ast-graph-builder? > > > > Nope because we also need to resolve the eval function for POSSIBLY_EVAL_CALL > > which we don't do with OTHER_CALL (there is no possibly_eval branch below like > > there is in ast-graph-builder / bytecode-graph-builder). > > > > Given that we never actually go through FCG for functions with eval any longer > > (since they will be optimized by Turbofan, so always go through Ignition after > > the change which feeds all TF code via Ignition) I don't think it's worth > doing > > any work here - we will eventually rip support for eval out of FCG once we are > > sure the TF via Ignition change is going to stick. > > I see. But it is still relevant in the ast-graph-builder though right? The ast-graph-builder will die when asm-js verification is enabled and we are sure the ignition for turbo change sticks. Nothing with eval should go through it right now, it is just an artifact of the testing infrastructure that some cctests still test it.
On 2016/11/10 10:08:53, rmcilroy wrote: > > > https://codereview.chromium.org/2487483004/diff/40001/src/full-codegen/full-c... > > src/full-codegen/full-codegen.cc:1672: case > > Call::POSSIBLY_EVAL_THROUGH_WITH_CALL: > > On 2016/11/09 17:02:29, rmcilroy wrote: > > > On 2016/11/09 14:13:08, Toon Verwaest wrote: > > > > Shouldn't POSSIBLY_EVAL_CALL be moved to OTHER_CALL now? Here and the > > > > ast-graph-builder? > > > > > > Nope because we also need to resolve the eval function for > POSSIBLY_EVAL_CALL > > > which we don't do with OTHER_CALL (there is no possibly_eval branch below > like > > > there is in ast-graph-builder / bytecode-graph-builder). > > > > > > Given that we never actually go through FCG for functions with eval any > longer > > > (since they will be optimized by Turbofan, so always go through Ignition > after > > > the change which feeds all TF code via Ignition) I don't think it's worth > > doing > > > any work here - we will eventually rip support for eval out of FCG once we > are > > > sure the TF via Ignition change is going to stick. > > > > I see. But it is still relevant in the ast-graph-builder though right? > > The ast-graph-builder will die when asm-js verification is enabled and we are > sure the ignition for turbo change sticks. Nothing with eval should go through > it right now, it is just an artifact of the testing infrastructure that some > cctests still test it. Sorry, maybe missed the point of your question. We can't change ast-graph-builder to be the same as bytecode-graph-builder because it deopts to full-codegen, and so needs to model the same kind of execution as full-codegen would do for these calls. As such, ast-graph-builder and FCG need to do the same thing for both types of Eval calls (this was the mistake I made in patch-set 1).
oh I see, that makes sense. lgtm then :)
Description was changed from ========== Only treat possible eval calls going through 'with' as special. This adds POSSIBLY_EVAL_THROUGH_WITH_CALL, which is the only case where we need to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. POSSIBLY_EVAL_CALL now just does a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ========== to ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of with blocks can now just does a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ==========
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15955) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15996) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12255) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28239)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16004) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/17509) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:120001) has been deleted
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15968) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16009) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM from my end. I agree, the code in ast-graph-builder is getting a bit more complex, but since it is being phased out I no longer have a strong opinion about it. Just a nit. Also s/does/do/ in the CL description I think. https://codereview.chromium.org/2487483004/diff/140001/src/compiler/ast-graph... File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/2487483004/diff/140001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2408: // Fall through. nit: Says "fall-through" but doesn't fall through AFAICT. Let's drop the comment.
Description was changed from ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of with blocks can now just does a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ========== to ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of 'with' blocks can now just do a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ==========
https://codereview.chromium.org/2487483004/diff/140001/src/compiler/ast-graph... File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/2487483004/diff/140001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2408: // Fall through. On 2016/11/10 15:10:22, Michael Starzinger wrote: > nit: Says "fall-through" but doesn't fall through AFAICT. Let's drop the > comment. oops, done thanks.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2487483004/#ps160001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/16099) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/12448)
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from verwaest@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2487483004/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of 'with' blocks can now just do a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ========== to ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of 'with' blocks can now just do a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of 'with' blocks can now just do a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 ========== to ========== Only treat possible eval calls going through 'with' as special. This removes the POSSIBLY_EVAL_CALL call type, and instead uses OTHER_CALL or WITH_CALL to decide whether to do the special LOOKUP_SLOT_CALL runtime call to find the callee and possibly update the receiver with the with-object. This means that eval calls out of 'with' blocks can now just do a normal LdaLookupGlobalSlot operation, which can check the context chain for eval extentions and fast-path the lookup if none exist. BUG=661556 Committed: https://crrev.com/be9b820c4405e3feb1bcc60f5bf257c574da76c9 Cr-Commit-Position: refs/heads/master@{#40965} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/be9b820c4405e3feb1bcc60f5bf257c574da76c9 Cr-Commit-Position: refs/heads/master@{#40965} |
