From 10883f561a6edb7f79896598e9b8cebb5c363fa6 Mon Sep 17 00:00:00 2001 From: Joshua Litt Date: Thu, 19 Sep 2019 09:08:07 -0700 Subject: [PATCH] [hole-check-elimination] Simplest possible hole check elimination doc: https://docs.google.com/document/d/1Y9uF3hS2aUrwKU56vGxlvEs_IiGgmWSzau8097Y-XBM/edit Bug: v8:7427 Change-Id: Iedd36c146cefff7e6687fdad48d263889c5c8347 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1778902 Commit-Queue: Ross McIlroy Reviewed-by: Ross McIlroy Reviewed-by: Toon Verwaest Cr-Commit-Position: refs/heads/master@{#63913} --- src/ast/scopes.cc | 4 + src/ast/scopes.h | 9 +- src/diagnostics/objects-printer.cc | 3 + src/interpreter/bytecode-generator.cc | 4 +- src/objects/scope-info.cc | 17 +- src/objects/scope-info.h | 5 + src/parsing/expression-scope.h | 19 ++- src/parsing/parser-base.h | 68 +++++++- src/parsing/preparse-data.cc | 23 ++- .../ClassAndSuperClass.golden | 14 +- .../SuperCallAndSpread.golden | 6 +- test/mjsunit/super_hole_check.mjs | 155 ++++++++++++++++++ 12 files changed, 288 insertions(+), 39 deletions(-) create mode 100644 test/mjsunit/super_hole_check.mjs diff --git a/src/ast/scopes.cc b/src/ast/scopes.cc index cf18a4ed6b..e165d2c616 100644 --- a/src/ast/scopes.cc +++ b/src/ast/scopes.cc @@ -195,6 +195,9 @@ DeclarationScope::DeclarationScope(Zone* zone, ScopeType scope_type, DCHECK(!is_eval_scope()); sloppy_eval_can_extend_vars_ = true; } + if (scope_info->CanElideThisHoleChecks()) { + can_elide_this_hole_checks_ = true; + } } Scope::Scope(Zone* zone, const AstRawString* catch_variable_name, @@ -228,6 +231,7 @@ void DeclarationScope::SetDefaults() { scope_uses_super_property_ = false; has_checked_syntax_ = false; has_this_reference_ = false; + can_elide_this_hole_checks_ = false; has_this_declaration_ = (is_function_scope() && !is_arrow_scope()) || is_module_scope(); needs_private_name_context_chain_recalc_ = false; diff --git a/src/ast/scopes.h b/src/ast/scopes.h index 3d1841a0e6..d28a2f2985 100644 --- a/src/ast/scopes.h +++ b/src/ast/scopes.h @@ -1083,11 +1083,13 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope { void set_has_this_reference() { has_this_reference_ = true; } bool has_this_reference() const { return has_this_reference_; } - void UsesThis() { - set_has_this_reference(); - GetReceiverScope()->receiver()->ForceContextAllocation(); + + bool can_elide_this_hole_checks() const { + return can_elide_this_hole_checks_; } + void set_can_elide_this_hole_checks() { can_elide_this_hole_checks_ = true; } + bool needs_private_name_context_chain_recalc() const { return needs_private_name_context_chain_recalc_; } @@ -1137,6 +1139,7 @@ class V8_EXPORT_PRIVATE DeclarationScope : public Scope { bool has_checked_syntax_ : 1; bool has_this_reference_ : 1; bool has_this_declaration_ : 1; + bool can_elide_this_hole_checks_ : 1; bool needs_private_name_context_chain_recalc_ : 1; // If the scope is a function scope, this is the function kind. diff --git a/src/diagnostics/objects-printer.cc b/src/diagnostics/objects-printer.cc index 547d2b8b9a..3d952987a0 100644 --- a/src/diagnostics/objects-printer.cc +++ b/src/diagnostics/objects-printer.cc @@ -2303,6 +2303,9 @@ void ScopeInfo::ScopeInfoPrint(std::ostream& os) { // NOLINT if (HasInferredFunctionName()) { os << "\n - inferred function name: " << Brief(InferredFunctionName()); } + if (CanElideThisHoleChecks()) { + os << "\n - can elide this hole checks"; + } if (HasPositionInfo()) { os << "\n - start position: " << StartPosition(); diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index 938b4e8291..da1cb79d34 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -5583,9 +5583,9 @@ void BytecodeGenerator::VisitTemplateLiteral(TemplateLiteral* expr) { void BytecodeGenerator::BuildThisVariableLoad() { DeclarationScope* receiver_scope = closure_scope()->GetReceiverScope(); Variable* var = receiver_scope->receiver(); - // TODO(littledan): implement 'this' hole check elimination. HoleCheckMode hole_check_mode = - IsDerivedConstructor(receiver_scope->function_kind()) + (IsDerivedConstructor(receiver_scope->function_kind()) && + !receiver_scope->can_elide_this_hole_checks()) ? HoleCheckMode::kRequired : HoleCheckMode::kElided; BuildVariableLoad(var, hole_check_mode); diff --git a/src/objects/scope-info.cc b/src/objects/scope-info.cc index 0f4dc26d99..1b9c2e5920 100644 --- a/src/objects/scope-info.cc +++ b/src/objects/scope-info.cc @@ -168,6 +168,7 @@ Handle ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope, bool has_simple_parameters = false; bool is_asm_module = false; bool sloppy_eval_can_extend_vars = false; + bool can_elide_this_hole_checks = false; if (scope->is_function_scope()) { DeclarationScope* function_scope = scope->AsDeclarationScope(); has_simple_parameters = function_scope->has_simple_parameters(); @@ -178,6 +179,8 @@ Handle ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope, function_kind = scope->AsDeclarationScope()->function_kind(); sloppy_eval_can_extend_vars = scope->AsDeclarationScope()->sloppy_eval_can_extend_vars(); + can_elide_this_hole_checks = + scope->AsDeclarationScope()->can_elide_this_hole_checks(); } // Encode the flags. @@ -199,7 +202,8 @@ Handle ScopeInfo::Create(Isolate* isolate, Zone* zone, Scope* scope, ForceContextAllocationField::encode( scope->ForceContextForLanguageMode()) | PrivateNameLookupSkipsOuterClassField::encode( - scope->private_name_lookup_skips_outer_class()); + scope->private_name_lookup_skips_outer_class()) | + CanElideThisHoleChecksField::encode(can_elide_this_hole_checks); scope_info.SetFlags(flags); scope_info.SetParameterCount(parameter_count); @@ -374,7 +378,8 @@ Handle ScopeInfo::CreateForWithScope( HasOuterScopeInfoField::encode(has_outer_scope_info) | IsDebugEvaluateScopeField::encode(false) | ForceContextAllocationField::encode(false) | - PrivateNameLookupSkipsOuterClassField::encode(false); + PrivateNameLookupSkipsOuterClassField::encode(false) | + CanElideThisHoleChecksField::encode(false); scope_info->SetFlags(flags); scope_info->SetParameterCount(0); @@ -441,7 +446,8 @@ Handle ScopeInfo::CreateForBootstrapping(Isolate* isolate, HasOuterScopeInfoField::encode(false) | IsDebugEvaluateScopeField::encode(false) | ForceContextAllocationField::encode(false) | - PrivateNameLookupSkipsOuterClassField::encode(false); + PrivateNameLookupSkipsOuterClassField::encode(false) | + CanElideThisHoleChecksField::encode(false); scope_info->SetFlags(flags); scope_info->SetParameterCount(parameter_count); scope_info->SetContextLocalCount(context_local_count); @@ -624,6 +630,11 @@ bool ScopeInfo::PrivateNameLookupSkipsOuterClass() const { return PrivateNameLookupSkipsOuterClassField::decode(Flags()); } +bool ScopeInfo::CanElideThisHoleChecks() const { + if (length() == 0) return false; + return CanElideThisHoleChecksField::decode(Flags()); +} + bool ScopeInfo::HasContext() const { return ContextLength() > 0; } Object ScopeInfo::FunctionName() const { diff --git a/src/objects/scope-info.h b/src/objects/scope-info.h index a8f483af0a..5c64457471 100644 --- a/src/objects/scope-info.h +++ b/src/objects/scope-info.h @@ -53,6 +53,9 @@ class ScopeInfo : public FixedArray { // Does this scope make a sloppy eval call? bool SloppyEvalCanExtendVars() const; + // True if we can elide 'this' hole checks in this scope. + bool CanElideThisHoleChecks() const; + // Return the number of context slots for code if a context is allocated. This // number consists of three parts: // 1. Size of fixed header for every context: Context::MIN_CONTEXT_SLOTS @@ -250,6 +253,8 @@ class ScopeInfo : public FixedArray { using ForceContextAllocationField = IsDebugEvaluateScopeField::Next; using PrivateNameLookupSkipsOuterClassField = ForceContextAllocationField::Next; + using CanElideThisHoleChecksField = + PrivateNameLookupSkipsOuterClassField::Next; STATIC_ASSERT(kLastFunctionKind <= FunctionKindField::kMax); diff --git a/src/parsing/expression-scope.h b/src/parsing/expression-scope.h index 43bf754150..c7221dd40a 100644 --- a/src/parsing/expression-scope.h +++ b/src/parsing/expression-scope.h @@ -127,6 +127,16 @@ class ExpressionScope { } while (scope != nullptr); } + void RecordCallsSuper() { + ExpressionScope* scope = this; + do { + if (scope->IsArrowHeadParsingScope()) { + scope->AsArrowHeadParsingScope()->RecordCallsSuper(); + } + scope = scope->parent(); + } while (scope != nullptr); + } + void RecordPatternError(const Scanner::Location& loc, MessageTemplate message) { // TODO(verwaest): Non-assigning expression? @@ -760,7 +770,12 @@ class ArrowHeadParsingScope : public ExpressionParsingScope { } #endif // DEBUG - if (uses_this_) result->UsesThis(); + if (uses_this_) { + result->set_has_this_reference(); + } + if (uses_this_ || calls_super_) { + result->GetReceiverScope()->receiver()->ForceContextAllocation(); + } return result; } @@ -773,6 +788,7 @@ class ArrowHeadParsingScope : public ExpressionParsingScope { void RecordNonSimpleParameter() { has_simple_parameter_list_ = false; } void RecordThisUse() { uses_this_ = true; } + void RecordCallsSuper() { calls_super_ = true; } private: FunctionKind kind() const { @@ -785,6 +801,7 @@ class ArrowHeadParsingScope : public ExpressionParsingScope { MessageTemplate declaration_error_message = MessageTemplate::kNone; bool has_simple_parameter_list_ = true; bool uses_this_ = false; + bool calls_super_ = false; DISALLOW_COPY_AND_ASSIGN(ArrowHeadParsingScope); }; diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h index 570202e6e7..3a9960a817 100644 --- a/src/parsing/parser-base.h +++ b/src/parsing/parser-base.h @@ -998,6 +998,21 @@ class ParserBase { return var; } + // Similar to UseThis, but does not disable hole check elision. + V8_INLINE void CallsSuper() { + DeclarationScope* closure_scope = scope()->GetClosureScope(); + DeclarationScope* receiver_scope = closure_scope->GetReceiverScope(); + if (closure_scope == receiver_scope) { + // It's possible that we're parsing the head of an arrow function, in + // which case we haven't realized yet that closure_scope != + // receiver_scope. Mark through the ExpressionScope for now. + expression_scope()->RecordCallsSuper(); + } else { + Variable* var = receiver_scope->receiver(); + var->ForceContextAllocation(); + } + } + V8_INLINE IdentifierT ParseAndClassifyIdentifier(Token::Value token); // Parses an identifier or a strict mode future reserved word. Allows passing // in function_kind for the case of parsing the identifier in a function @@ -1091,6 +1106,8 @@ class ParserBase { ExpressionT ParseArrowFunctionLiteral(const FormalParametersT& parameters); void ParseAsyncFunctionBody(Scope* scope, StatementListT* body); + void ParseDerivedConstructorBody(StatementListT* body, + Token::Value end_token); ExpressionT ParseAsyncFunctionLiteral(); ExpressionT ParseClassLiteral(IdentifierT name, Scanner::Location class_name_location, @@ -3473,8 +3490,7 @@ typename ParserBase::ExpressionT ParserBase::ParseSuperExpression( if (!is_new && peek() == Token::LPAREN && IsDerivedConstructor(kind)) { // TODO(rossberg): This might not be the correct FunctionState for the // method here. - expression_scope()->RecordThisUse(); - UseThis(); + CallsSuper(); return impl()->NewSuperCallReference(pos); } } @@ -4045,16 +4061,18 @@ void ParserBase::ParseFunctionBody( impl()->ParseAndRewriteGeneratorFunctionBody(pos, kind, &inner_body); } else if (IsAsyncFunction(kind)) { ParseAsyncFunctionBody(inner_scope, &inner_body); + } else if (IsDerivedConstructor(kind)) { + ParseDerivedConstructorBody(&inner_body, closing_token); + { + ExpressionParsingScope expression_scope(impl()); + inner_body.Add(factory()->NewReturnStatement(impl()->ThisExpression(), + kNoSourcePosition)); + expression_scope.ValidateExpression(); + } } else { ParseStatementList(&inner_body, closing_token); } - if (IsDerivedConstructor(kind)) { - ExpressionParsingScope expression_scope(impl()); - inner_body.Add(factory()->NewReturnStatement(impl()->ThisExpression(), - kNoSourcePosition)); - expression_scope.ValidateExpression(); - } Expect(closing_token); } } @@ -4712,6 +4730,40 @@ typename ParserBase::ExpressionT ParserBase::ParseV8Intrinsic() { return impl()->NewV8Intrinsic(name, args, pos); } +template +void ParserBase::ParseDerivedConstructorBody(StatementListT* body, + Token::Value end_token) { + // Allocate a target stack to use for this set of source elements. This way, + // all scripts and functions get their own target stack thus avoiding illegal + // breaks and continues across functions. + TargetScopeT target_scope(this); + while (peek() != end_token) { + StatementT stat = impl()->NullStatement(); + // We can elide some hole checks in a derived constructor with a top level + // 'super,' but only if 'this' has not been used before the super. For + // safety we also require hole checks if 'eval' was called before 'super.' + if (V8_UNLIKELY(peek() == Token::SUPER && PeekAhead() == Token::LPAREN) && + !GetReceiverScope()->receiver()->is_used() && + !GetReceiverScope()->inner_scope_calls_eval()) { + int pos = peek_position(); + ExpressionT expr = ParseExpression(); + + // Check again to confirm the 'super' expression did not reference + // 'this' or use 'eval.' + DeclarationScope* receiver_scope = GetReceiverScope(); + if (!receiver_scope->receiver()->is_used() && + !receiver_scope->inner_scope_calls_eval()) { + receiver_scope->set_can_elide_this_hole_checks(); + } + stat = factory()->NewExpressionStatement(expr, pos); + } else { + stat = ParseStatementListItem(); + } + if (impl()->IsNull(stat)) return; + if (stat->IsEmptyStatement()) continue; + body->Add(stat); + } +} template void ParserBase::ParseStatementList(StatementListT* body, diff --git a/src/parsing/preparse-data.cc b/src/parsing/preparse-data.cc index bc4235eded..b1a1b5af41 100644 --- a/src/parsing/preparse-data.cc +++ b/src/parsing/preparse-data.cc @@ -26,6 +26,8 @@ using InnerScopeCallsEvalField = ScopeSloppyEvalCanExtendVarsField::Next; using NeedsPrivateNameContextChainRecalcField = InnerScopeCallsEvalField::Next; +using CanElideThisHoleChecks = + NeedsPrivateNameContextChainRecalcField::Next; using VariableMaybeAssignedField = BitField8; using VariableContextAllocatedField = VariableMaybeAssignedField::Next; @@ -354,7 +356,7 @@ void PreparseDataBuilder::SaveDataForScope(Scope* scope) { byte_data_.WriteUint8(scope->scope_type()); #endif - uint8_t eval_and_private_recalc = + uint8_t scope_data_flags = ScopeSloppyEvalCanExtendVarsField::encode( scope->is_declaration_scope() && scope->AsDeclarationScope()->sloppy_eval_can_extend_vars()) | @@ -362,9 +364,12 @@ void PreparseDataBuilder::SaveDataForScope(Scope* scope) { NeedsPrivateNameContextChainRecalcField::encode( scope->is_function_scope() && scope->AsDeclarationScope() - ->needs_private_name_context_chain_recalc()); + ->needs_private_name_context_chain_recalc()) | + CanElideThisHoleChecks::encode( + scope->is_declaration_scope() && + scope->AsDeclarationScope()->can_elide_this_hole_checks()); byte_data_.Reserve(kUint8Size); - byte_data_.WriteUint8(eval_and_private_recalc); + byte_data_.WriteUint8(scope_data_flags); if (scope->is_function_scope()) { Variable* function = scope->AsDeclarationScope()->function_var(); @@ -605,17 +610,19 @@ void BaseConsumedPreparseData::RestoreDataForScope(Scope* scope) { DCHECK_EQ(scope_data_->ReadUint8(), scope->scope_type()); CHECK(scope_data_->HasRemainingBytes(ByteData::kUint8Size)); - uint32_t eval_and_private_recalc = scope_data_->ReadUint8(); - if (ScopeSloppyEvalCanExtendVarsField::decode(eval_and_private_recalc)) { + uint32_t scope_data_flags = scope_data_->ReadUint8(); + if (ScopeSloppyEvalCanExtendVarsField::decode(scope_data_flags)) { scope->RecordEvalCall(); } - if (InnerScopeCallsEvalField::decode(eval_and_private_recalc)) { + if (InnerScopeCallsEvalField::decode(scope_data_flags)) { scope->RecordInnerScopeEvalCall(); } - if (NeedsPrivateNameContextChainRecalcField::decode( - eval_and_private_recalc)) { + if (NeedsPrivateNameContextChainRecalcField::decode(scope_data_flags)) { scope->AsDeclarationScope()->RecordNeedsPrivateNameContextChainRecalc(); } + if (CanElideThisHoleChecks::decode(scope_data_flags)) { + scope->AsDeclarationScope()->set_can_elide_this_hole_checks(); + } if (scope->is_function_scope()) { Variable* function = scope->AsDeclarationScope()->function_var(); diff --git a/test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden b/test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden index 1dd2c099eb..5178e7d1c9 100644 --- a/test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden +++ b/test/cctest/interpreter/bytecode_expectations/ClassAndSuperClass.golden @@ -106,7 +106,7 @@ snippet: " " frame size: 6 parameter count: 1 -bytecode array length: 40 +bytecode array length: 36 bytecodes: [ B(Mov), R(closure), R(1), /* 113 E> */ B(StackCheck), @@ -120,12 +120,9 @@ bytecodes: [ B(Ldar), R(this), B(ThrowSuperAlreadyCalledIfNotHole), B(Mov), R(5), R(this), - /* 128 S> */ B(Ldar), R(this), - B(ThrowSuperNotCalledIfHole), - B(LdaSmi), I8(2), + /* 128 S> */ B(LdaSmi), I8(2), /* 136 E> */ B(StaNamedProperty), R(this), U8(0), U8(2), B(Ldar), R(this), - B(ThrowSuperNotCalledIfHole), /* 141 S> */ B(Return), ] constant pool: [ @@ -149,7 +146,7 @@ snippet: " " frame size: 5 parameter count: 1 -bytecode array length: 36 +bytecode array length: 32 bytecodes: [ B(Mov), R(closure), R(1), /* 112 E> */ B(StackCheck), @@ -161,12 +158,9 @@ bytecodes: [ B(Ldar), R(this), B(ThrowSuperAlreadyCalledIfNotHole), B(Mov), R(4), R(this), - /* 126 S> */ B(Ldar), R(this), - B(ThrowSuperNotCalledIfHole), - B(LdaSmi), I8(2), + /* 126 S> */ B(LdaSmi), I8(2), /* 134 E> */ B(StaNamedProperty), R(this), U8(0), U8(2), B(Ldar), R(this), - B(ThrowSuperNotCalledIfHole), /* 139 S> */ B(Return), ] constant pool: [ diff --git a/test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden b/test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden index 2ec0a8baa5..3e0d7189db 100644 --- a/test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden +++ b/test/cctest/interpreter/bytecode_expectations/SuperCallAndSpread.golden @@ -51,7 +51,7 @@ snippet: " " frame size: 9 parameter count: 1 -bytecode array length: 40 +bytecode array length: 39 bytecodes: [ B(CreateRestParameter), B(Star), R(3), @@ -70,7 +70,6 @@ bytecodes: [ B(ThrowSuperAlreadyCalledIfNotHole), B(Mov), R(8), R(this), B(Ldar), R(this), - B(ThrowSuperNotCalledIfHole), /* 159 S> */ B(Return), ] constant pool: [ @@ -93,7 +92,7 @@ snippet: " " frame size: 11 parameter count: 1 -bytecode array length: 112 +bytecode array length: 111 bytecodes: [ B(CreateRestParameter), B(Star), R(3), @@ -138,7 +137,6 @@ bytecodes: [ B(ThrowSuperAlreadyCalledIfNotHole), B(Mov), R(9), R(this), B(Ldar), R(this), - B(ThrowSuperNotCalledIfHole), /* 162 S> */ B(Return), ] constant pool: [ diff --git a/test/mjsunit/super_hole_check.mjs b/test/mjsunit/super_hole_check.mjs new file mode 100644 index 0000000000..a703e53bdb --- /dev/null +++ b/test/mjsunit/super_hole_check.mjs @@ -0,0 +1,155 @@ +// Copyright 2019 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +new class extends Object { + constructor() { + super(); this.foo = 1; + } +} + +{ + class Throws extends Object { + t = 1; + constructor(t = this.t) { + super(); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + this.x = true; + super(); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super(this); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super(eval("this")); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super(eval("(() => this)()")); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor(t = eval("this")) { + super(); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super(); + super(); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + if (false) { super(); } + this.x = true; + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super((4 * this.t)); + } + } + assertThrows(() => new Throws, ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super((() => this)()); + } + } + assertThrows(() => new Throws(), ReferenceError); +} + +{ + class Throws extends Object { + constructor() { + super((()=>{ var x = ()=> this; return x(); })()) + } + } + assertThrows(() => new Throws(), ReferenceError); +} + +{ + class C extends null { + constructor() { + super(); + } + } + assertThrows(() => new C(), TypeError); +} + +{ + class C extends Object { + constructor() { + super(); + (() => { + this; + })(); + } + } + new C(); +} + +{ + var count = 0; + + class A { + constructor() { + count++; + } + increment() { + count++; + } + } + + class B extends A { + constructor() { + super(); + (_ => super.increment())(); + } + } + new B(); + assertEquals(count, 2); +}