Skip to content

Commit

Permalink
Force context allocation for variables in generator scopes.
Browse files Browse the repository at this point in the history
* src/scopes.h (ForceContextAllocation, has_forced_context_allocation):
  New interface to force context allocation for an entire function's
  scope.

* src/scopes.cc: Unless a new scope is a function scope, if its outer
  scope has forced context allocation, it should also force context
  allocation.
  (MustAllocateInContext): Return true if the scope as a whole has
  forced context allocation.
  (CollectStackAndContextLocals): Allow temporaries to be
  context-allocated.

* src/parser.cc (ParseFunctionLiteral): Force context allocation for
  generator scopes.

* src/v8globals.h (VariableMode): Update comment on TEMPORARY.

* src/arm/full-codegen-arm.cc (Generate):
* src/ia32/full-codegen-ia32.cc (Generate):
* src/x64/full-codegen-x64.cc (Generate): Assert that generators have no
  stack slots.

* test/mjsunit/harmony/generators-instantiation.js: New test.

BUG=v8:2355
TEST=mjsunit/harmony/generators-instantiation

Review URL: https://codereview.chromium.org/13408005
Patch from Andy Wingo <wingo@igalia.com>.

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@14152 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
  • Loading branch information
mstarzinger@chromium.org committed Apr 5, 2013
1 parent 647ec2c commit 4e58a8e
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 9 deletions.
5 changes: 3 additions & 2 deletions src/arm/full-codegen-arm.cc
Expand Up @@ -162,8 +162,6 @@ void FullCodeGenerator::Generate() {
// the frame (that is done below).
FrameScope frame_scope(masm_, StackFrame::MANUAL);

int locals_count = info->scope()->num_stack_slots();

info->set_prologue_offset(masm_->pc_offset());
{
PredictableCodeSizeScope predictible_code_size_scope(
Expand All @@ -179,6 +177,9 @@ void FullCodeGenerator::Generate() {
}

{ Comment cmnt(masm_, "[ Allocate locals");
int locals_count = info->scope()->num_stack_slots();
// Generators allocate locals, if any, in context slots.
ASSERT(!info->function()->is_generator() || locals_count == 0);
for (int i = 0; i < locals_count; i++) {
__ push(ip);
}
Expand Down
2 changes: 2 additions & 0 deletions src/ia32/full-codegen-ia32.cc
Expand Up @@ -164,6 +164,8 @@ void FullCodeGenerator::Generate() {

{ Comment cmnt(masm_, "[ Allocate locals");
int locals_count = info->scope()->num_stack_slots();
// Generators allocate locals, if any, in context slots.
ASSERT(!info->function()->is_generator() || locals_count == 0);
if (locals_count == 1) {
__ push(Immediate(isolate()->factory()->undefined_value()));
} else if (locals_count > 1) {
Expand Down
3 changes: 3 additions & 0 deletions src/parser.cc
Expand Up @@ -4391,6 +4391,9 @@ FunctionLiteral* Parser::ParseFunctionLiteral(Handle<String> function_name,
// Parse function body.
{ FunctionState function_state(this, scope, is_generator, isolate());
top_scope_->SetScopeName(function_name);
// For generators, allocating variables in contexts is currently a win
// because it minimizes the work needed to suspend and resume an activation.
if (is_generator) top_scope_->ForceContextAllocation();

// FormalParameterList ::
// '(' (Identifier)*[','] ')'
Expand Down
21 changes: 16 additions & 5 deletions src/scopes.cc
Expand Up @@ -197,6 +197,8 @@ void Scope::SetDefaults(ScopeType type,
outer_scope_calls_non_strict_eval_ = false;
inner_scope_calls_eval_ = false;
force_eager_compilation_ = false;
force_context_allocation_ = (outer_scope != NULL && !is_function_scope())
? outer_scope->has_forced_context_allocation() : false;
num_var_or_const_ = 0;
num_stack_slots_ = 0;
num_heap_slots_ = 0;
Expand Down Expand Up @@ -603,12 +605,18 @@ void Scope::CollectStackAndContextLocals(ZoneList<Variable*>* stack_locals,
}
}

// Collect temporaries which are always allocated on the stack.
// Collect temporaries which are always allocated on the stack, unless the
// context as a whole has forced context allocation.
for (int i = 0; i < temps_.length(); i++) {
Variable* var = temps_[i];
if (var->is_used()) {
ASSERT(var->IsStackLocal());
stack_locals->Add(var, zone());
if (var->IsContextSlot()) {
ASSERT(has_forced_context_allocation());
context_locals->Add(var, zone());
} else {
ASSERT(var->IsStackLocal());
stack_locals->Add(var, zone());
}
}
}

Expand Down Expand Up @@ -1182,8 +1190,11 @@ bool Scope::MustAllocateInContext(Variable* var) {
// an eval() call or a runtime with lookup), it must be allocated in the
// context.
//
// Exceptions: temporary variables are never allocated in a context;
// catch-bound variables are always allocated in a context.
// Exceptions: If the scope as a whole has forced context allocation, all
// variables will have context allocation, even temporaries. Otherwise
// temporary variables are always stack-allocated. Catch-bound variables are
// always context-allocated.
if (has_forced_context_allocation()) return true;
if (var->mode() == TEMPORARY) return false;
if (var->mode() == INTERNAL) return true;
if (is_catch_scope() || is_block_scope() || is_module_scope()) return true;
Expand Down
10 changes: 10 additions & 0 deletions src/scopes.h
Expand Up @@ -269,6 +269,15 @@ class Scope: public ZoneObject {
end_position_ = statement_pos;
}

// In some cases we want to force context allocation for a whole scope.
void ForceContextAllocation() {
ASSERT(!already_resolved());
force_context_allocation_ = true;
}
bool has_forced_context_allocation() const {
return force_context_allocation_;
}

// ---------------------------------------------------------------------------
// Predicates.

Expand Down Expand Up @@ -494,6 +503,7 @@ class Scope: public ZoneObject {
bool outer_scope_calls_non_strict_eval_;
bool inner_scope_calls_eval_;
bool force_eager_compilation_;
bool force_context_allocation_;

// True if it doesn't need scope resolution (e.g., if the scope was
// constructed based on a serialized scope info or a catch context).
Expand Down
4 changes: 2 additions & 2 deletions src/v8globals.h
Expand Up @@ -496,8 +496,8 @@ enum VariableMode {
INTERNAL, // like VAR, but not user-visible (may or may not
// be in a context)

TEMPORARY, // temporary variables (not user-visible), never
// in a context
TEMPORARY, // temporary variables (not user-visible), stack-allocated
// unless the scope as a whole has forced context allocation

DYNAMIC, // always require dynamic lookup (we don't know
// the declaration)
Expand Down
2 changes: 2 additions & 0 deletions src/x64/full-codegen-x64.cc
Expand Up @@ -160,6 +160,8 @@ void FullCodeGenerator::Generate() {

{ Comment cmnt(masm_, "[ Allocate locals");
int locals_count = info->scope()->num_stack_slots();
// Generators allocate locals, if any, in context slots.
ASSERT(!info->function()->is_generator() || locals_count == 0);
if (locals_count == 1) {
__ PushRoot(Heap::kUndefinedValueRootIndex);
} else if (locals_count > 1) {
Expand Down
49 changes: 49 additions & 0 deletions test/mjsunit/harmony/generators-instantiation.js
@@ -0,0 +1,49 @@
// Copyright 2013 the V8 project authors. All rights reserved.
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are
// met:
//
// * Redistributions of source code must retain the above copyright
// notice, this list of conditions and the following disclaimer.
// * Redistributions in binary form must reproduce the above
// copyright notice, this list of conditions and the following
// disclaimer in the documentation and/or other materials provided
// with the distribution.
// * Neither the name of Google Inc. nor the names of its
// contributors may be used to endorse or promote products derived
// from this software without specific prior written permission.
//
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

// Flags: --harmony-generators --harmony-scoping

// Test instantations of generators.

// Generators shouldn't allocate stack slots. This test will abort in debug
// mode if generators have stack slots.
function TestContextAllocation() {
function* g1(a, b, c) { yield 1; return [a, b, c]; }
function* g2() { yield 1; return arguments; }
function* g3() { yield 1; return this; }
function* g4() { var x = 10; yield 1; return x; }
// Temporary variable context allocation
function* g5(l) { "use strict"; yield 1; for (let x in l) { yield x; } }

g1();
g2();
g3();
g4();
g5(["foo"]);
}

TestContextAllocation();

0 comments on commit 4e58a8e

Please sign in to comment.