Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 2 months ago by rmcilroy Modified:
5 years, 2 months ago Reviewers:
adamk 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. |
DescriptionAdd support for calculating a scopes maximum nested context chain.
Adds Scope::MaxNestedContextChainLength() which calculates the maximum length
of the context chain for the given scope. This is used by the interpreter to
preallocate the approprate number of context registers when compiling the
function.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/d820268c0ba9536f3289206e2d928aa7e9436037
Cr-Commit-Position: refs/heads/master@{#31309}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Rebase #
Created: 5 years, 2 months ago
Dependent Patchsets: Messages
Total messages: 16 (5 generated)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1404793004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1404793004/20001
rmcilroy@chromium.org changed reviewers: + adamk@chromium.org
Adam, could you take a look please. I'm not very failure with the parser - I think this is right, but would appreciate any advice you have. PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:155: builder()->set_context_count(scope()->MaxNestedContextChainLength()); Is this exercised by any tests (I'm not familiar with the testing plan for the interpreter)? https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:158: if (scope()->NeedsContext()) { I'd leave this as num_heap_slots() > 0 (see other comment). https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h#newcode348 src/scopes.h:348: return is_with_scope() || num_heap_slots() > 0; The other callers of NeedsContext() only call it on a block scope, so this is sort of weird. Maybe change them to do the num_heap_slots() check directly, and make this method private on Scope?
https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:155: builder()->set_context_count(scope()->MaxNestedContextChainLength()); On 2015/10/15 07:38:44, adamk wrote: > Is this exercised by any tests (I'm not familiar with the testing plan for the > interpreter)? This is exercised by test-bytecode-generator.cc::DeclareGlobals and test-interpreter.cc::DeclareGlobals, but only for the inner function context (just below) since we don't yet have support for any other nested contexts (this change is required before we can add that support). One thing I was trying to figure out - is there any way to trigger a normal (non-with) block to require a nested context without calling eval (which we don't support yet either)? If there is an easy way to do this which is supported by the current interpreter functionality I could add a test for that to this CL. https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:158: if (scope()->NeedsContext()) { On 2015/10/15 07:38:45, adamk wrote: > I'd leave this as num_heap_slots() > 0 (see other comment). Commented on the other comment. https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h#newcode348 src/scopes.h:348: return is_with_scope() || num_heap_slots() > 0; On 2015/10/15 07:38:45, adamk wrote: > The other callers of NeedsContext() only call it on a block scope, so this is > sort of weird. Maybe change them to do the num_heap_slots() check directly, and > make this method private on Scope? I could do this, however personally I think it is a bit weird that NeedsContext is only valid for block scopes - is there any particular reason for this that I'm missing? As for making this private - it is called by Fullcodegen, Crankshaft and TF, so I would need to change a bunch of other places if I made this private.
https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:155: builder()->set_context_count(scope()->MaxNestedContextChainLength()); On 2015/10/15 09:23:07, rmcilroy wrote: > On 2015/10/15 07:38:44, adamk wrote: > > Is this exercised by any tests (I'm not familiar with the testing plan for the > > interpreter)? > > This is exercised by test-bytecode-generator.cc::DeclareGlobals and > test-interpreter.cc::DeclareGlobals, but only for the inner function context > (just below) since we don't yet have support for any other nested contexts (this > change is required before we can add that support). > > One thing I was trying to figure out - is there any way to trigger a normal > (non-with) block to require a nested context without calling eval (which we > don't support yet either)? If there is an easy way to do this which is supported > by the current interpreter functionality I could add a test for that to this CL. > Capturing a lexical variable from a block will force it to be heap allocated: function f() { "use strict"; let x = 1; { let y = 2; return function g() { return [x, y]; } } } in this case, g's context chain will include the block containing "y". https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h#newcode348 src/scopes.h:348: return is_with_scope() || num_heap_slots() > 0; On 2015/10/15 09:23:07, rmcilroy wrote: > On 2015/10/15 07:38:45, adamk wrote: > > The other callers of NeedsContext() only call it on a block scope, so this is > > sort of weird. Maybe change them to do the num_heap_slots() check directly, > and > > make this method private on Scope? > > I could do this, however personally I think it is a bit weird that NeedsContext > is only valid for block scopes - is there any particular reason for this that > I'm missing? As for making this private - it is called by Fullcodegen, > Crankshaft and TF, so I would need to change a bunch of other places if I made > this private. What I'm saying is that testing "is_with_scope()" for cases where the code knows it has a block scope (or in the case of your other caller, a function scope) seems a bit odd. But I guess it doesn't cause any problems, so I'll take your perspective as someone new to this code as advise here, and since this additional check won't cause any problems, it's fine.
https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1404793004/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:155: builder()->set_context_count(scope()->MaxNestedContextChainLength()); On 2015/10/15 09:34:32, adamk wrote: > On 2015/10/15 09:23:07, rmcilroy wrote: > > On 2015/10/15 07:38:44, adamk wrote: > > > Is this exercised by any tests (I'm not familiar with the testing plan for > the > > > interpreter)? > > > > This is exercised by test-bytecode-generator.cc::DeclareGlobals and > > test-interpreter.cc::DeclareGlobals, but only for the inner function context > > (just below) since we don't yet have support for any other nested contexts > (this > > change is required before we can add that support). > > > > One thing I was trying to figure out - is there any way to trigger a normal > > (non-with) block to require a nested context without calling eval (which we > > don't support yet either)? If there is an easy way to do this which is > supported > > by the current interpreter functionality I could add a test for that to this > CL. > > > > Capturing a lexical variable from a block will force it to be heap allocated: > > function f() { > "use strict"; > let x = 1; > { > let y = 2; > return function g() { return [x, y]; } > } > } > > in this case, g's context chain will include the block containing "y". Great, thanks for the pointer. It turns out this needs a bit more support than the interpreter supported, but I've added the test to my followup CL at https://codereview.chromium.org/1403943004/ (both in test-interpreter.cc and test-bytecode-generator.cc). https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h File src/scopes.h (right): https://codereview.chromium.org/1404793004/diff/20001/src/scopes.h#newcode348 src/scopes.h:348: return is_with_scope() || num_heap_slots() > 0; On 2015/10/15 09:34:32, adamk wrote: > On 2015/10/15 09:23:07, rmcilroy wrote: > > On 2015/10/15 07:38:45, adamk wrote: > > > The other callers of NeedsContext() only call it on a block scope, so this > is > > > sort of weird. Maybe change them to do the num_heap_slots() check directly, > > and > > > make this method private on Scope? > > > > I could do this, however personally I think it is a bit weird that > NeedsContext > > is only valid for block scopes - is there any particular reason for this that > > I'm missing? As for making this private - it is called by Fullcodegen, > > Crankshaft and TF, so I would need to change a bunch of other places if I made > > this private. > > What I'm saying is that testing "is_with_scope()" for cases where the code knows > it has a block scope (or in the case of your other caller, a function scope) > seems a bit odd. But I guess it doesn't cause any problems, so I'll take your > perspective as someone new to this code as advise here, and since this > additional check won't cause any problems, it's fine. Acknowledged.
lgtm
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1404793004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1404793004/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/d820268c0ba9536f3289206e2d928aa7e9436037 Cr-Commit-Position: refs/heads/master@{#31309} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
