Separate Scope into DeclarationScope and Scope

Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209
Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Committed: https://crrev.com/ff1c3cdb18be28031bd531ade9beb4f97c159816
Cr-Original-Commit-Position: refs/heads/master@{#38367}
Cr-Commit-Position: refs/heads/master@{#38390}

[Toon Verwaest, 2016-08-03 12:56:07]

verwaest@chromium.org changed reviewers:
+ ahaas@chromium.org, marja@chromium.org, mstarzinger@chromium.org

[Toon Verwaest, 2016-08-03 12:56:08]

marja: ptal
mstarzinger: ptal interpreter and compiler
ahaas: ptal asmjs

[Toon Verwaest, 2016-08-03 12:56:57]

Description was changed from

==========
Separate Scope into DeclarationScope and Scope

BUG=v8:5209
==========

to

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209
==========

[Toon Verwaest, 2016-08-03 13:00:36]

cc adamk

[Michael Starzinger, 2016-08-03 13:02:19]

https://codereview.chromium.org/2209573002/diff/120001/src/compiler/ast-graph...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2209573002/diff/120001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:718: Scope* scope,
nit: Can we change the signature to take "DeclarationScope" instead of "Scope",
then we don't need to cast the scope below.

https://codereview.chromium.org/2209573002/diff/120001/src/compiler/ast-graph...
src/compiler/ast-graph-builder.cc:750: GetDebugParameterName(graph()->zone(),
scope->AsDeclarationScope(), i);
nit: See comment above.

https://codereview.chromium.org/2209573002/diff/120001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2209573002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:659: DCHECK_EQ(scope(), info()->scope());
nit: Can we change BytecodeGenerator::scope() to return a "DeclarationScope"
instead of a "Scope" and leave the rest of this function untouched?

https://codereview.chromium.org/2209573002/diff/120001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:3020: DeclarationScope* scope =
this->scope()->AsDeclarationScope();
nit: See comment above.

[ahaas, 2016-08-03 13:06:40]

On 2016/08/03 at 13:02:19, mstarzinger wrote:
>
https://codereview.chromium.org/2209573002/diff/120001/src/compiler/ast-graph...
> File src/compiler/ast-graph-builder.cc (right):
> 
>
https://codereview.chromium.org/2209573002/diff/120001/src/compiler/ast-graph...
> src/compiler/ast-graph-builder.cc:718: Scope* scope,
> nit: Can we change the signature to take "DeclarationScope" instead of
"Scope", then we don't need to cast the scope below.
> 
>
https://codereview.chromium.org/2209573002/diff/120001/src/compiler/ast-graph...
> src/compiler/ast-graph-builder.cc:750: GetDebugParameterName(graph()->zone(),
scope->AsDeclarationScope(), i);
> nit: See comment above.
> 
>
https://codereview.chromium.org/2209573002/diff/120001/src/interpreter/byteco...
> File src/interpreter/bytecode-generator.cc (right):
> 
>
https://codereview.chromium.org/2209573002/diff/120001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:659: DCHECK_EQ(scope(),
info()->scope());
> nit: Can we change BytecodeGenerator::scope() to return a "DeclarationScope"
instead of a "Scope" and leave the rest of this function untouched?
> 
>
https://codereview.chromium.org/2209573002/diff/120001/src/interpreter/byteco...
> src/interpreter/bytecode-generator.cc:3020: DeclarationScope* scope =
this->scope()->AsDeclarationScope();
> nit: See comment above.

asm.js lgtm

[Toon Verwaest, 2016-08-03 13:24:32]

Addressed compiler / interpreter comments

[Michael Starzinger, 2016-08-03 13:29:02]

LGTM on "compiler" and "interpreter". Didn't look at the rest.

[adamk, 2016-08-04 00:18:49]

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

[adamk, 2016-08-04 00:18:51]

A few high-level comments/questions.

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:35: if (scope->is_declaration_scope() &&
Code like this reads strangely. "this" declarations don't have much to do with
"declaration scopes" conceptually, so it's weird that the caller has to know
things about the scope hierarchy before asking a question like "is there a
receiver" in this scope?

Did you consider doing something more like what you did for AstNodes and
teaching the base class more about its subclass?

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:51: scope->is_declaration_scope() &&
Same here. I suspect I'd make this comment in a lot of places. Would be
interested to hear your thoughts. I like the memory savings, but making the
calling code strange is something I'd like to avoid.

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h#newco...
src/ast/scopes.h:678: class DeclarationScope : public Scope {
Please give this a class comment. "DeclarationScope" is a name that I know
causes initial confusion to anyone encountering it, and it seems like this CL
actually increases that confusion, since there's stuff in DeclarationScope that
has nothing to do with the essential thing about DeclarationScopes, which is
that they host "var" declarations.

For that matter, I think this means that Scope now needs a comment, explaining
which scopes can be represented by a Scope and which need a DeclarationScope.

https://codereview.chromium.org/2209573002/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (left):

https://codereview.chromium.org/2209573002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:4947: inner_scope = inner_scope->FinalizeBlockScope();
I don't think it's worth changing this, there's some advantage to any further
attempts to touch a finalized scope crashing instead of continuing silently (it
also seems unrelated to this patch).

[marja, 2016-08-04 07:58:08]

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h#newco...
src/ast/scopes.h:164: ZoneList<Declaration*>* declarations() { return &decls_; }
Why is this in Scope, not in DeclarationScope?

[Toon Verwaest, 2016-08-04 08:28:15]

You would never guess, but is_declaration_scope is quite unrelated to whether it
has any declarations ... :) DeclarationScope is top-level, closurescope, or
varblock. They can have vars, temps, ... Lexical declarations, which are mixed
into decls_ / variables_, can show up in any scope.

[Toon Verwaest, 2016-08-04 12:01:17]

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:35: if (scope->is_declaration_scope() &&
On 2016/08/04 00:18:50, adamk wrote:
> Code like this reads strangely. "this" declarations don't have much to do with
> "declaration scopes" conceptually, so it's weird that the caller has to know
> things about the scope hierarchy before asking a question like "is there a
> receiver" in this scope?
> 
> Did you consider doing something more like what you did for AstNodes and
> teaching the base class more about its subclass?

The reasoning there is quite different than here though. It's the base visitor
that knows about the actual visitor, and there's only one visitor that's doing
the work. The nodes are "virtual" and they didn't change at all. Here I'm
introducing distinction between Scopes just like ast nodes are distinct and need
to be handled as such by visitors.

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:51: scope->is_declaration_scope() &&
On 2016/08/04 00:18:51, adamk wrote:
> Same here. I suspect I'd make this comment in a lot of places. Would be
> interested to hear your thoughts. I like the memory savings, but making the
> calling code strange is something I'd like to avoid.

I agree that we shouldn't make calling code strange for unrelated implementation
details. However, I think it's important for callers to know what they are
doing. We shouldn't have tons of helper methods that figure out what they are
doing automagically, since it's confuses code. Not having a receiver() at all on
Scope makes it clear that it doesn't have a receiver, and that invariant is
clear from the design. I'd prefer coming back to this code later and more
clearly separate Scope from DeclarationScope handling.

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h#newco...
src/ast/scopes.h:678: class DeclarationScope : public Scope {
On 2016/08/04 00:18:51, adamk wrote:
> Please give this a class comment. "DeclarationScope" is a name that I know
> causes initial confusion to anyone encountering it, and it seems like this CL
> actually increases that confusion, since there's stuff in DeclarationScope
that
> has nothing to do with the essential thing about DeclarationScopes, which is
> that they host "var" declarations.
> 
> For that matter, I think this means that Scope now needs a comment, explaining
> which scopes can be represented by a Scope and which need a DeclarationScope.

I don't think it increases confusion. All scopes covered by this class are
declaration scopes, and are not scopes. DeclarationScope specializes (reduces
the scope of) those scopes, hence it's more clear when we could be in such
scopes (only when it's a declaration scope) and when not (not declaration
scope). This gives clients more static guarantees about being in such scopes;
reducing the error-potential.

We should probably create more subclasses of DeclarationScope though. (One for
each of the constructor wrappers; NewScriptScope, NewModuleScope, NewEvalScope,
NewVarblockScope, NewFunctionScope.)

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h#newco...
src/ast/scopes.h:678: class DeclarationScope : public Scope {
On 2016/08/04 00:18:51, adamk wrote:
> Please give this a class comment. "DeclarationScope" is a name that I know
> causes initial confusion to anyone encountering it, and it seems like this CL
> actually increases that confusion, since there's stuff in DeclarationScope
that
> has nothing to do with the essential thing about DeclarationScopes, which is
> that they host "var" declarations.
> 
> For that matter, I think this means that Scope now needs a comment, explaining
> which scopes can be represented by a Scope and which need a DeclarationScope.

Acknowledged.

https://codereview.chromium.org/2209573002/diff/140001/src/parsing/parser.cc
File src/parsing/parser.cc (left):

https://codereview.chromium.org/2209573002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:4947: inner_scope = inner_scope->FinalizeBlockScope();
On 2016/08/04 00:18:51, adamk wrote:
> I don't think it's worth changing this, there's some advantage to any further
> attempts to touch a finalized scope crashing instead of continuing silently
(it
> also seems unrelated to this patch).

inner_scope is a DeclarationScope, and FinalizeBlockScope returns a Scope*. I
now explicitly null out inner_scope to guarantee we don't use it at all
afterwards.

[marja, 2016-08-04 12:18:17]

(Since adamk@ is already reviewing, relaying this CL to him. I had a quick look
at the parser parts and LG.)

[adamk, 2016-08-04 16:31:28]

I wonder what better name there could be than
"DeclarationScope"..."VarDeclarationScope"? It's really unclear out of context
that "Var" there refers explicitly to "var" declarations, not variables in
general, so I'm not sure it really helps. Marja, any naming thoughts?

I'm still not loving this intermediate state, but it sounds like this is just
step 1 of a further split of the Scope hierarchy. As I note below, I think the
callers will look better once we actually have separate FunctionScope
VarblockScope, etc. So lgtm to unblock future work here (and let you land this
before you need any more merges :)

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:35: if (scope->is_declaration_scope() &&
On 2016/08/04 12:01:17, Toon Verwaest wrote:
> On 2016/08/04 00:18:50, adamk wrote:
> > Code like this reads strangely. "this" declarations don't have much to do
with
> > "declaration scopes" conceptually, so it's weird that the caller has to know
> > things about the scope hierarchy before asking a question like "is there a
> > receiver" in this scope?
> > 
> > Did you consider doing something more like what you did for AstNodes and
> > teaching the base class more about its subclass?
> 
> The reasoning there is quite different than here though. It's the base visitor
> that knows about the actual visitor, and there's only one visitor that's doing
> the work. The nodes are "virtual" and they didn't change at all. Here I'm
> introducing distinction between Scopes just like ast nodes are distinct and
need
> to be handled as such by visitors.

What I'm saying is that "has_this_declaration()" is a question that doesn't
apply any more to varblock scopes than it does to block scopes, yet it works
just fine when called on varblocks but doesn't even exist (to return false) on
blocks.

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:51: scope->is_declaration_scope() &&
On 2016/08/04 12:01:17, Toon Verwaest wrote:
> On 2016/08/04 00:18:51, adamk wrote:
> > Same here. I suspect I'd make this comment in a lot of places. Would be
> > interested to hear your thoughts. I like the memory savings, but making the
> > calling code strange is something I'd like to avoid.
> 
> I agree that we shouldn't make calling code strange for unrelated
implementation
> details. However, I think it's important for callers to know what they are
> doing. We shouldn't have tons of helper methods that figure out what they are
> doing automagically, since it's confuses code. Not having a receiver() at all
on
> Scope makes it clear that it doesn't have a receiver, and that invariant is
> clear from the design. I'd prefer coming back to this code later and more
> clearly separate Scope from DeclarationScope handling.

Again, receiver()-having doesn't make any more sense for DeclarationScopes than
it does for Scopes at large.

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopes.h#newco...
src/ast/scopes.h:678: class DeclarationScope : public Scope {
On 2016/08/04 12:01:17, Toon Verwaest wrote:
> On 2016/08/04 00:18:51, adamk wrote:
> > Please give this a class comment. "DeclarationScope" is a name that I know
> > causes initial confusion to anyone encountering it, and it seems like this
CL
> > actually increases that confusion, since there's stuff in DeclarationScope
> that
> > has nothing to do with the essential thing about DeclarationScopes, which is
> > that they host "var" declarations.
> > 
> > For that matter, I think this means that Scope now needs a comment,
explaining
> > which scopes can be represented by a Scope and which need a
DeclarationScope.
> 
> I don't think it increases confusion. All scopes covered by this class are
> declaration scopes, and are not scopes. DeclarationScope specializes (reduces
> the scope of) those scopes, hence it's more clear when we could be in such
> scopes (only when it's a declaration scope) and when not (not declaration
> scope). This gives clients more static guarantees about being in such scopes;
> reducing the error-potential.
> 
> We should probably create more subclasses of DeclarationScope though. (One for
> each of the constructor wrappers; NewScriptScope, NewModuleScope,
NewEvalScope,
> NewVarblockScope, NewFunctionScope.)

I think this last paragraph is what makes me feel like this is going in the
right direction, even if this intermediate step looks weird to me. If folks
asking for a receiver had to check for some subclass of scopes _all of which_
had a receiver, or all of which have parameters, then I think this new layout
does provide nice understandable static guarantees.

[marja, 2016-08-05 07:00:38]

src/ast/* and src/parsering/* lgtm

https://codereview.chromium.org/2209573002/diff/160001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2209573002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:1079: : kNormalFunction;
It's slightly weird that we pass kNormalFunction if it's not a function.
Fortunately it's only printing code.

[Toon Verwaest, 2016-08-05 08:41:09]

Ok, it sounds like we are in agreement on the direction. I'll rebase and land. I
agree that this isn't optimal yet, but this CL is already large and complex
enough by itself; I think it's an ok checkpoint that's not worse than what we
had before :)

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2209573002/diff/140001/src/ast/scopeinfo.cc#n...
src/ast/scopeinfo.cc:35: if (scope->is_declaration_scope() &&
On 2016/08/04 16:31:28, adamk wrote:
> On 2016/08/04 12:01:17, Toon Verwaest wrote:
> > On 2016/08/04 00:18:50, adamk wrote:
> > > Code like this reads strangely. "this" declarations don't have much to do
> with
> > > "declaration scopes" conceptually, so it's weird that the caller has to
know
> > > things about the scope hierarchy before asking a question like "is there a
> > > receiver" in this scope?
> > > 
> > > Did you consider doing something more like what you did for AstNodes and
> > > teaching the base class more about its subclass?
> > 
> > The reasoning there is quite different than here though. It's the base
visitor
> > that knows about the actual visitor, and there's only one visitor that's
doing
> > the work. The nodes are "virtual" and they didn't change at all. Here I'm
> > introducing distinction between Scopes just like ast nodes are distinct and
> need
> > to be handled as such by visitors.
> 
> What I'm saying is that "has_this_declaration()" is a question that doesn't
> apply any more to varblock scopes than it does to block scopes, yet it works
> just fine when called on varblocks but doesn't even exist (to return false) on
> blocks.

Ok, I do agree to that. But at least this is a step forward towards only having
it available where relevant :)

[marja, 2016-08-05 09:10:55]

Re: naming, I see adamk@'s point wrt DeclarationScope / VarDeclarationScope. I
don't have a preference there. VarDeclarationScope is somehow clearer, but only
if you already know what it means :P DeclarationScope is shorter and
sufficiently clear.

[Toon Verwaest, 2016-08-05 09:19:09]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-05 09:19:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
mstarzinger@chromium.org, adamk@chromium.org, marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2209573002/#ps200001
(title: "Fix rebase issue")

[commit-bot: I haz the power, 2016-08-05 09:19:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 09:22:26]

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

[commit-bot: I haz the power, 2016-08-05 09:22:27]

Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6385)

[Toon Verwaest, 2016-08-05 09:41:37]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-05 09:41:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, ahaas@chromium.org, adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2209573002/#ps220001
(title: "Add ports")

[commit-bot: I haz the power, 2016-08-05 09:41:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 09:45:19]

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

[commit-bot: I haz the power, 2016-08-05 09:45:22]

Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[Toon Verwaest, 2016-08-05 09:47:10]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-05 09:47:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, ahaas@chromium.org, adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2209573002/#ps240001
(title: "blah")

[commit-bot: I haz the power, 2016-08-05 09:47:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 10:15:04]

Description was changed from

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209
==========

to

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209
==========

[commit-bot: I haz the power, 2016-08-05 10:15:05]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-08-05 10:18:00]

Description was changed from

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209
==========

to

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}
==========

[commit-bot: I haz the power, 2016-08-05 10:18:01]

Patchset 13 (id:??) landed as
https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}

[Michael Achenbach, 2016-08-05 12:05:39]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2212383003/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Make leak checker unhappy:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20debug/buil....

[Toon Verwaest, 2016-08-05 13:40:31]

Description was changed from

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}
==========

to

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}
==========

[Toon Verwaest, 2016-08-05 13:40:46]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-05 13:40:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, ahaas@chromium.org, adamk@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2209573002/#ps260001
(title: "Move has_simple_parameters_ to DeclarationScope")

[commit-bot: I haz the power, 2016-08-05 13:40:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-08-05 13:42:13]

Relanding since moving has_simple_parameters to DeclarationScope makes the
warning go away locally. Lets see if the bots are happy with it.

https://codereview.chromium.org/2209573002/diff/260001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2209573002/diff/260001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:201: 
Here just shuffling around to put in the same order as in the header file.

https://codereview.chromium.org/2209573002/diff/260001/src/ast/scopes.h
File src/ast/scopes.h (right):

https://codereview.chromium.org/2209573002/diff/260001/src/ast/scopes.h#newco...
src/ast/scopes.h:880: bool has_simple_parameters_ : 1;
This is the actual change that valgrind was falling over. Otherwise it wasn't
initialized for some scopes.

[commit-bot: I haz the power, 2016-08-05 14:30:59]

Description was changed from

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}
==========

to

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}
==========

[commit-bot: I haz the power, 2016-08-05 14:31:02]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-05 14:34:17]

Description was changed from

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Cr-Commit-Position: refs/heads/master@{#38367}
==========

to

==========
Separate Scope into DeclarationScope and Scope

This reduces peak zone memory usage by ~10% on codeload and ~5% on mandreel.

BUG=v8:5209

Committed: https://crrev.com/2648162dcfff622f8587cea2faa8c3af56456968
Committed: https://crrev.com/ff1c3cdb18be28031bd531ade9beb4f97c159816
Cr-Original-Commit-Position: refs/heads/master@{#38367}
Cr-Commit-Position: refs/heads/master@{#38390}
==========

[commit-bot: I haz the power, 2016-08-05 14:34:18]

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