Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 4 months ago by Michael Starzinger Modified:
4 years, 4 months ago Reviewers:
rmcilroy CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[interpreter] Allow mixed stacks if bytecode is preserved.
This changes the compilation pipeline so that mixed stacks are allowed
when bytecode is preserved. This means there can be activations of both,
"baseline" as well as "unoptimized" code active on the stack at the same
time for any single given function.
R=rmcilroy@chromium.org
BUG=v8:4280
Committed: https://crrev.com/5e08f43531b5f2d8577c6e523c49911b2347db66
Cr-Commit-Position: refs/heads/master@{#38809}
Patch Set 1 #Patch Set 2 : Fix comment. #Patch Set 3 : Adapt test. #
Total comments: 8
Patch Set 4 : Addressed comment. #Patch Set 5 : One more comment. #
Created: 4 years, 4 months ago
Messages
Total messages: 19 (13 generated)
The CQ bit was checked by mstarzinger@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 checked by mstarzinger@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. Will be interesting to see if this has any impact on the perf bots. Thanks. https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc#newcode951 src/compiler.cc:951: // underlying bytecode is cleared below. nit - could you update the comment to mention we only do this if FLAG_ignition_preserve_bytecode isn't set https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc#newcode... src/compiler.cc:1421: // can be sure there are no activations. And here https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc#newcode... src/compiler.cc:1441: // that it can be used as the "source of truth" eventually. nit - could you update the comment https://codereview.chromium.org/2267693002/diff/40001/test/cctest/test-compil... File test/cctest/test-compiler.cc (right): https://codereview.chromium.org/2267693002/diff/40001/test/cctest/test-compil... test/cctest/test-compiler.cc:806: bool base = FLAG_ignition_preserve_bytecode; just do: "if (FLAG_ignition_preserve_bytecode) return" up at the top?
The CQ bit was checked by mstarzinger@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...
Thanks! Comments addressed. Will land tomorrow. https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc#newcode951 src/compiler.cc:951: // underlying bytecode is cleared below. On 2016/08/22 15:33:03, rmcilroy wrote: > nit - could you update the comment to mention we only do this if > FLAG_ignition_preserve_bytecode isn't set Done. https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc#newcode... src/compiler.cc:1421: // can be sure there are no activations. On 2016/08/22 15:33:03, rmcilroy wrote: > And here Done. https://codereview.chromium.org/2267693002/diff/40001/src/compiler.cc#newcode... src/compiler.cc:1441: // that it can be used as the "source of truth" eventually. On 2016/08/22 15:33:03, rmcilroy wrote: > nit - could you update the comment Done. https://codereview.chromium.org/2267693002/diff/40001/test/cctest/test-compil... File test/cctest/test-compiler.cc (right): https://codereview.chromium.org/2267693002/diff/40001/test/cctest/test-compil... test/cctest/test-compiler.cc:806: bool base = FLAG_ignition_preserve_bytecode; On 2016/08/22 15:33:03, rmcilroy wrote: > just do: "if (FLAG_ignition_preserve_bytecode) return" up at the top? Done.
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 mstarzinger@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/2267693002/#ps80001 (title: "One more comment.")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [interpreter] Allow mixed stacks if bytecode is preserved. This changes the compilation pipeline so that mixed stacks are allowed when bytecode is preserved. This means there can be activations of both, "baseline" as well as "unoptimized" code active on the stack at the same time for any single given function. R=rmcilroy@chromium.org BUG=v8:4280 ========== to ========== [interpreter] Allow mixed stacks if bytecode is preserved. This changes the compilation pipeline so that mixed stacks are allowed when bytecode is preserved. This means there can be activations of both, "baseline" as well as "unoptimized" code active on the stack at the same time for any single given function. R=rmcilroy@chromium.org BUG=v8:4280 Committed: https://crrev.com/5e08f43531b5f2d8577c6e523c49911b2347db66 Cr-Commit-Position: refs/heads/master@{#38809} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/5e08f43531b5f2d8577c6e523c49911b2347db66 Cr-Commit-Position: refs/heads/master@{#38809} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
