Add support for calculating a scopes maximum nested context chain.

Add 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}

[rmcilroy, 2015-10-14 16:41:00]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[rmcilroy, 2015-10-14 16:41:05]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2015-10-14 16:41:07]

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, 2015-10-14 16:42:11]

rmcilroy@chromium.org changed reviewers:
+ adamk@chromium.org

[rmcilroy, 2015-10-14 16:42:11]

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.

[commit-bot: I haz the power, 2015-10-14 17:34:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-14 17:34:52]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-10-15 07:38:45]

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?

[rmcilroy, 2015-10-15 09:23:07]

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.

[adamk, 2015-10-15 09:34:32]

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.

[rmcilroy, 2015-10-15 14:06:16]

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.

[adamk, 2015-10-15 14:16:59]

lgtm

[rmcilroy, 2015-10-15 14:36:13]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-10-15 14:36:22]

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

[commit-bot: I haz the power, 2015-10-15 15:44:00]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-10-15 15:44:25]

Patchset 2 (id:??) landed as
https://crrev.com/d820268c0ba9536f3289206e2d928aa7e9436037
Cr-Commit-Position: refs/heads/master@{#31309}