Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2437593003:
[IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. (Closed)
Description[IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames.
Some cleanup after the cl to fix --trace-ic to work with ignition
(https://codereview.chromium.org/2405173007/). In GetSharedFunctionInfo,
we used to skip the bytecode handler frame, which is no longer required.
BUG=v8:4280
Committed: https://crrev.com/1ec9526c7aa5cab8bedcc14ded49f3f2de72e37d
Cr-Commit-Position: refs/heads/master@{#40762}
Patch Set 1 #Patch Set 2 : Rebased the patch and removed FLAG_ignition from constructor. #
Total comments: 8
Patch Set 3 : Addresses comments from Ross. #Messages
Total messages: 42 (31 generated)
The CQ bit was checked by mythria@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.
The CQ bit was checked by mythria@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_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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15435) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15465) 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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15400) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27576)
The CQ bit was checked by mythria@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.
Description was changed from ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after this cl: https://codereview.chromium.org/2405173007/ to fix --trace-ic to work with ignition. The code to skip the bytecode handler frame is conditionally done on FLAG_ignition. Also, in GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ========== to ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after this cl: https://codereview.chromium.org/2405173007/ to fix --trace-ic to work with ignition. In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ==========
Description was changed from ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after this cl: https://codereview.chromium.org/2405173007/ to fix --trace-ic to work with ignition. In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ========== to ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after this cl to fix --trace-ic to work with ignition: https://codereview.chromium.org/2405173007/ In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ==========
Description was changed from ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after this cl to fix --trace-ic to work with ignition: https://codereview.chromium.org/2405173007/ In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ========== to ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after the cl to fix --trace-ic to work with ignition (https://codereview.chromium.org/2405173007/). In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ==========
mythria@chromium.org changed reviewers: + leszeks@chromium.org, rmcilroy@chromium.org
I should have sent this out last week, but somehow missed it. In IC::GetSharedFunctionInfo, we no longer need to skip frames for interpreted functions. We already skip this frame in the constructor. PTAL. https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc File src/ic/ic.cc (right): https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode195 src/ic/ic.cc:195: // TODO(rmcilroy): Remove this once bytecode handlers don't need a frame. Do we need this TODO? since we need to call into runtime for this handler, is it possible to avoid a frame? https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode258 src/ic/ic.cc:258: DCHECK(it.frame()->type() != StackFrame::STUB); I wanted to add a check to see if it is the correct type, but that has a lot of frame types: Interpreted, optimized, javascript, builtin and may be ArgumentsAdaptor.
LGTM https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc File src/ic/ic.cc (right): https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode195 src/ic/ic.cc:195: // TODO(rmcilroy): Remove this once bytecode handlers don't need a frame. On 2016/10/31 15:47:37, mythria wrote: > Do we need this TODO? since we need to call into runtime for this handler, is it > possible to avoid a frame? Yeah I had plans to avoid needing to build the frame, but it's probably not worth it. Could you just remove this TODO please. https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode203 src/ic/ic.cc:203: Intended changes? https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode258 src/ic/ic.cc:258: DCHECK(it.frame()->type() != StackFrame::STUB); On 2016/10/31 15:47:37, mythria wrote: > I wanted to add a check to see if it is the correct type, but that has a lot of > frame types: Interpreted, optimized, javascript, builtin and may be > ArgumentsAdaptor. Hmm, we could probably just remove this - I'm not sure we need to check it isn't a STUB frame.
Thanks Ross. Fixed them. https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc File src/ic/ic.cc (right): https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode195 src/ic/ic.cc:195: // TODO(rmcilroy): Remove this once bytecode handlers don't need a frame. On 2016/11/01 13:01:02, rmcilroy (OOO - slow) wrote: > On 2016/10/31 15:47:37, mythria wrote: > > Do we need this TODO? since we need to call into runtime for this handler, is > it > > possible to avoid a frame? > > Yeah I had plans to avoid needing to build the frame, but it's probably not > worth it. Could you just remove this TODO please. Done. https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode203 src/ic/ic.cc:203: On 2016/11/01 13:01:02, rmcilroy (OOO - slow) wrote: > Intended changes? No. Removed them. Done. https://codereview.chromium.org/2437593003/diff/20001/src/ic/ic.cc#newcode258 src/ic/ic.cc:258: DCHECK(it.frame()->type() != StackFrame::STUB); On 2016/11/01 13:01:02, rmcilroy (OOO - slow) wrote: > On 2016/10/31 15:47:37, mythria wrote: > > I wanted to add a check to see if it is the correct type, but that has a lot > of > > frame types: Interpreted, optimized, javascript, builtin and may be > > ArgumentsAdaptor. > > Hmm, we could probably just remove this - I'm not sure we need to check it isn't > a STUB frame. Removed it. I wasn't very happy with that check either.
The CQ bit was checked by mythria@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.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2437593003/#ps40001 (title: "Addresses comments from Ross.")
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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27836)
mythria@chromium.org changed reviewers: + ishell@chromium.org
Hi Igor, We used to skip bytecode handler frames in IC::GetSharedFunctionInfo. We no longer need it, because this logic moved to the constructor in this cl: https://codereview.chromium.org/2405173007/ Could you please take a look. Thanks, Mythri
lgtm
Thanks Igor.
The CQ bit was checked by mythria@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.
The CQ bit was checked by mythria@chromium.org
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 ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after the cl to fix --trace-ic to work with ignition (https://codereview.chromium.org/2405173007/). In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ========== to ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after the cl to fix --trace-ic to work with ignition (https://codereview.chromium.org/2405173007/). In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after the cl to fix --trace-ic to work with ignition (https://codereview.chromium.org/2405173007/). In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 ========== to ========== [IC] IC::GetSharedFunctionInfo does not need to skip bytecode handler frames. Some cleanup after the cl to fix --trace-ic to work with ignition (https://codereview.chromium.org/2405173007/). In GetSharedFunctionInfo, we used to skip the bytecode handler frame, which is no longer required. BUG=v8:4280 Committed: https://crrev.com/1ec9526c7aa5cab8bedcc14ded49f3f2de72e37d Cr-Commit-Position: refs/heads/master@{#40762} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1ec9526c7aa5cab8bedcc14ded49f3f2de72e37d Cr-Commit-Position: refs/heads/master@{#40762} |
