Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917193007:
[compiler] Guard implicit tier-up when ensuring deopt support. (Closed)
|
Created:
4 years, 7 months ago by Michael Starzinger Modified:
4 years, 7 months ago 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[compiler] Guard implicit tier-up when ensuring deopt support.
This makes sure that Compiler::EnsureDeoptimizationSupport follows the
same limitations as other compilation functions that trigger a tier-up.
Specifically it prevents against tier-up while inlining when activations
are present on the stack.
R=yangguo@chromium.org
BUG=chromium:607494
LOG=n
Committed: https://crrev.com/d9462d04a0fcc3a8adaa8cf92c1da30a8f03061e
Cr-Commit-Position: refs/heads/master@{#35923}
Patch Set 1 #Patch Set 2 : Rebased. #Patch Set 3 : Rebased. #Patch Set 4 : Fix accidental tier-up. #Patch Set 5 : Rebased. #
Total comments: 7
Messages
Total messages: 22 (9 generated)
On 2016/04/27 16:11:24, Michael Starzinger wrote: As discussed, there is one last failure to fix. And having already landed https://codereview.chromium.org/1921853005/, you will run into some rebase conflicts. Sorry.
On 2016/04/28 06:42:06, Yang wrote: > On 2016/04/27 16:11:24, Michael Starzinger wrote: > > As discussed, there is one last failure to fix. And having already landed > https://codereview.chromium.org/1921853005/, you will run into some rebase > conflicts. Sorry. Yep, not a problem. Will tackle the last failure today and ping you as soon as I have a new version. Thanks!
Patchset #3 (id:30001) has been deleted
PTAL. Ready for review.
Description was changed from ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org ========== to ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org ==========
Description was changed from ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org ========== to ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org ==========
mstarzinger@chromium.org changed reviewers: + rmcilroy@chromium.org
mstarzinger@chromium.org changed reviewers: + rmcilroy@chromium.org
+Ross.
LGTM, thanks.
LGTM. Comments and questions. https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode863 src/compiler.cc:863: public OptimizedFunctionVisitor { Multiple inheritance. I hope that's fine with the style guide? (I haven't checked). There is actually no need to mix these two, right? Also, is there a way to abort visiting optimized functions if we already found an activation? https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode881 src/compiler.cc:881: if (function->Inlines(shared_)) has_activations_ = true; We could be more precise here and only mark has_activations_ if the optimized code actually deopts to bytecode. https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode... src/compiler.cc:1380: shared->EnableDeoptimizationSupport(*unoptimized.code()); Is there a reason this has been moved to after installing scope info?
https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode863 src/compiler.cc:863: public OptimizedFunctionVisitor { On 2016/04/29 10:19:49, Yang wrote: > Multiple inheritance. I hope that's fine with the style guide? (I haven't > checked). There is actually no need to mix these two, right? Also, is there a > way to abort visiting optimized functions if we already found an activation? Both classes are pure virtual, which is allowed by the style guide: https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance
Description was changed from ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org ========== to ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org BUG=chromium:607494 LOG=n ==========
https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc File src/compiler.cc (right): https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode863 src/compiler.cc:863: public OptimizedFunctionVisitor { On 2016/04/29 10:50:29, rmcilroy wrote: > On 2016/04/29 10:19:49, Yang wrote: > > Multiple inheritance. I hope that's fine with the style guide? (I haven't > > checked). There is actually no need to mix these two, right? Also, is there a > > way to abort visiting optimized functions if we already found an activation? > > Both classes are pure virtual, which is allowed by the style guide: > https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance I don't have a strong opinion on this. Happy to separate it into two classes. Either way is fine with me. https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode881 src/compiler.cc:881: if (function->Inlines(shared_)) has_activations_ = true; On 2016/04/29 10:19:49, Yang wrote: > We could be more precise here and only mark has_activations_ if the optimized > code actually deopts to bytecode. Not sure I understand entirely. The only way optimized code could exist without the SharedFunctionInfo being in tier 2, is when it has been generated directly from tier 1. That means all deopt points building a frame for {shared} pretty much are guaranteed to be interpreter frames. So IMHO the check is already as precise as it can be. Conversely in the --no-turbo-from-bytecode case we are guaranteed to not have any optimized code inlining the SharedFunctionInfo in question. Maybe I am missing something here. https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode... src/compiler.cc:1380: shared->EnableDeoptimizationSupport(*unoptimized.code()); On 2016/04/29 10:19:49, Yang wrote: > Is there a reason this has been moved to after installing scope info? No strong reason other than to bring the steps in the same order as in the other compile functions (i.e. GetUnoptimizedCode and GetBaselineCode). Eventually I would like to unify all these copies of the pipeline into one.
On 2016/04/29 10:59:10, Michael Starzinger wrote: > https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc > File src/compiler.cc (right): > > https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode863 > src/compiler.cc:863: public OptimizedFunctionVisitor { > On 2016/04/29 10:50:29, rmcilroy wrote: > > On 2016/04/29 10:19:49, Yang wrote: > > > Multiple inheritance. I hope that's fine with the style guide? (I haven't > > > checked). There is actually no need to mix these two, right? Also, is there > a > > > way to abort visiting optimized functions if we already found an activation? > > > > Both classes are pure virtual, which is allowed by the style guide: > > https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance > > I don't have a strong opinion on this. Happy to separate it into two classes. > Either way is fine with me. > > https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode881 > src/compiler.cc:881: if (function->Inlines(shared_)) has_activations_ = true; > On 2016/04/29 10:19:49, Yang wrote: > > We could be more precise here and only mark has_activations_ if the optimized > > code actually deopts to bytecode. > > Not sure I understand entirely. The only way optimized code could exist without > the SharedFunctionInfo being in tier 2, is when it has been generated directly > from tier 1. That means all deopt points building a frame for {shared} pretty > much are guaranteed to be interpreter frames. So IMHO the check is already as > precise as it can be. Conversely in the --no-turbo-from-bytecode case we are > guaranteed to not have any optimized code inlining the SharedFunctionInfo in > question. Maybe I am missing something here. > > https://codereview.chromium.org/1917193007/diff/80001/src/compiler.cc#newcode... > src/compiler.cc:1380: shared->EnableDeoptimizationSupport(*unoptimized.code()); > On 2016/04/29 10:19:49, Yang wrote: > > Is there a reason this has been moved to after installing scope info? > > No strong reason other than to bring the steps in the same order as in the other > compile functions (i.e. GetUnoptimizedCode and GetBaselineCode). Eventually I > would like to unify all these copies of the pipeline into one. I see. LGTM. Thanks for the explanations.
The CQ bit was checked by mstarzinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917193007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917193007/80001
Message was sent while issue was closed.
Description was changed from ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org BUG=chromium:607494 LOG=n ========== to ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org BUG=chromium:607494 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org BUG=chromium:607494 LOG=n ========== to ========== [compiler] Guard implicit tier-up when ensuring deopt support. This makes sure that Compiler::EnsureDeoptimizationSupport follows the same limitations as other compilation functions that trigger a tier-up. Specifically it prevents against tier-up while inlining when activations are present on the stack. R=yangguo@chromium.org BUG=chromium:607494 LOG=n Committed: https://crrev.com/d9462d04a0fcc3a8adaa8cf92c1da30a8f03061e Cr-Commit-Position: refs/heads/master@{#35923} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d9462d04a0fcc3a8adaa8cf92c1da30a8f03061e Cr-Commit-Position: refs/heads/master@{#35923} |
