Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2156753002:
[Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode (Closed)
|
Created:
4 years, 5 months ago by rmcilroy Modified:
4 years, 5 months ago Reviewers:
Michael Starzinger 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[Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode
Always use the BytecodeGraphBuilder when the --turbo-from-bytecode
is enabled, assuming the function should be compiled for Ignition.
Adds a new MaybeOptimizeIgnition function to runtime-profiler
which is called if the function should be optimized from bytecode
rather than going via full-codegen.
BUG=v8:4280
Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4
Committed: https://crrev.com/a474e84181fb6b19cc8c48cab29d670bf6302ab7
Cr-Original-Commit-Position: refs/heads/master@{#37921}
Cr-Commit-Position: refs/heads/master@{#38002}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix regress-446389 and remove DCHECK from ast-graph-builder. #
Total comments: 8
Patch Set 3 : Review comments #
Messages
Total messages: 50 (37 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: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Description was changed from ========== NOT FOR REVIEW ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. BUG=v8:4280 ==========
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_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/10681) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
Patchset #1 (id:20001) has been deleted
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...
Description was changed from ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. BUG=v8:4280 ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/9244) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
rmcilroy@chromium.org changed reviewers: + mstarzinger@chromium.org
Michi, this CL still needs tweaking (I will fix the issue with regress-446389 on the bots), but just wondering what you think of the general direction. Does this seem reasonable overall? Happy to change the APIs in Compiler that I've added as you would prefer.
Overall approach looks reasonable at a first glance. Just one high-level comment that jumps to mind. https://codereview.chromium.org/2156753002/diff/40001/src/compiler.h File src/compiler.h (right): https://codereview.chromium.org/2156753002/diff/40001/src/compiler.h#newcode78 src/compiler.h:78: static bool ShouldUseIgnition(CompilationInfo* info); There seems to be only one use of this predicate outside the compiler and it is inside an assertion. Could we instead move the assertion from the {AstGraphBuilder} into {GetOptimizedCode} where we decide to call {info->MarkAsOptimizeFromBytecode} or not? I really would like to avoid exposing this predicate.
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...
PTAL, thanks. https://codereview.chromium.org/2156753002/diff/40001/src/compiler.h File src/compiler.h (right): https://codereview.chromium.org/2156753002/diff/40001/src/compiler.h#newcode78 src/compiler.h:78: static bool ShouldUseIgnition(CompilationInfo* info); On 2016/07/19 09:36:02, Michael Starzinger wrote: > There seems to be only one use of this predicate outside the compiler and it is > inside an assertion. Could we instead move the assertion from the > {AstGraphBuilder} into {GetOptimizedCode} where we decide to call > {info->MarkAsOptimizeFromBytecode} or not? I really would like to avoid exposing > this predicate. Yeah I thought you might say this :). I've removed the assertion - there isn't really any other assertion we can add to GetOptimizedCode, it should mark it unconditinally now (or bail out of optimization if EnsureBytecode fails), so this should be just as good.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Just nits. Thanks! https://codereview.chromium.org/2156753002/diff/60001/src/compiler.h File src/compiler.h (right): https://codereview.chromium.org/2156753002/diff/60001/src/compiler.h#newcode69 src/compiler.h:69: // Ensures that bytecode is generated. nit: We could add "..., calls ParseAndAnalyze internally." to the comment to clarify that it is not required for the caller to do that. https://codereview.chromium.org/2156753002/diff/60001/src/compiler.h#newcode73 src/compiler.h:73: // optimization. nit: We could add "This is used as a hint by the profiler." to clarify that the predicate is intended to be just a hint for users of this API. https://codereview.chromium.org/2156753002/diff/60001/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2156753002/diff/60001/src/runtime-profiler.cc... src/runtime-profiler.cc:116: // a bytecode array. nit: The TODO applied to the assertion I think, let's also drop the TODO. https://codereview.chromium.org/2156753002/diff/60001/test/cctest/cctest.status File test/cctest/cctest.status (right): https://codereview.chromium.org/2156753002/diff/60001/test/cctest/cctest.stat... test/cctest/cctest.status:415: # immutable in the BytecodeGraphBuilder, therefore no inlining happens. Also in addition to the reason mentioned in this comment, inlining in general is disabled (see InliningPhase::Run all the way at the bottom). The reason is that inlining is hard-coded to use the AstGraphBuilder, so we decided to just disable it for now.
https://codereview.chromium.org/2156753002/diff/60001/src/compiler.h File src/compiler.h (right): https://codereview.chromium.org/2156753002/diff/60001/src/compiler.h#newcode69 src/compiler.h:69: // Ensures that bytecode is generated. On 2016/07/19 16:00:07, Michael Starzinger wrote: > nit: We could add "..., calls ParseAndAnalyze internally." to the comment to > clarify that it is not required for the caller to do that. Done. https://codereview.chromium.org/2156753002/diff/60001/src/compiler.h#newcode73 src/compiler.h:73: // optimization. On 2016/07/19 16:00:07, Michael Starzinger wrote: > nit: We could add "This is used as a hint by the profiler." to clarify that the > predicate is intended to be just a hint for users of this API. Done. https://codereview.chromium.org/2156753002/diff/60001/src/runtime-profiler.cc File src/runtime-profiler.cc (right): https://codereview.chromium.org/2156753002/diff/60001/src/runtime-profiler.cc... src/runtime-profiler.cc:116: // a bytecode array. On 2016/07/19 16:00:07, Michael Starzinger wrote: > nit: The TODO applied to the assertion I think, let's also drop the TODO. Done. https://codereview.chromium.org/2156753002/diff/60001/test/cctest/cctest.status File test/cctest/cctest.status (right): https://codereview.chromium.org/2156753002/diff/60001/test/cctest/cctest.stat... test/cctest/cctest.status:415: # immutable in the BytecodeGraphBuilder, therefore no inlining happens. On 2016/07/19 16:00:08, Michael Starzinger wrote: > Also in addition to the reason mentioned in this comment, inlining in general is > disabled (see InliningPhase::Run all the way at the bottom). The reason is that > inlining is hard-coded to use the AstGraphBuilder, so we decided to just disable > it for now. Done.
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.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2156753002/#ps80001 (title: "Review comments")
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 ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:80001) has been created in https://codereview.chromium.org/2165223002/ by machenbach@chromium.org. The reason for reverting is: Breaks tsan: https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/10758.
Message was sent while issue was closed.
Description was changed from ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921} ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921} ==========
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.
Relanding after fixing race in TF.
The CQ bit was checked by rmcilroy@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 ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921} ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Cr-Commit-Position: refs/heads/master@{#37921} ========== to ========== [Intepreter] Always use BytecodeGraphBuilder when --turbo-from-bytecode Always use the BytecodeGraphBuilder when the --turbo-from-bytecode is enabled, assuming the function should be compiled for Ignition. Adds a new MaybeOptimizeIgnition function to runtime-profiler which is called if the function should be optimized from bytecode rather than going via full-codegen. BUG=v8:4280 Committed: https://crrev.com/9ca7db914be88e6792a88eab4a1988ee031d70c4 Committed: https://crrev.com/a474e84181fb6b19cc8c48cab29d670bf6302ab7 Cr-Original-Commit-Position: refs/heads/master@{#37921} Cr-Commit-Position: refs/heads/master@{#38002} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a474e84181fb6b19cc8c48cab29d670bf6302ab7 Cr-Commit-Position: refs/heads/master@{#38002} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
