From 83f60ab5ac10f5a7b20321bb872df0ef00b9dc73 Mon Sep 17 00:00:00 2001 From: cbruni Date: Thu, 5 Nov 2015 11:21:06 -0800 Subject: [PATCH] [crankshaft] Do not optimize ClassConstructor calls and apply. LOG=N BUG=v8:4428 Review URL: https://codereview.chromium.org/1425293007 Cr-Commit-Position: refs/heads/master@{#31839} --- src/crankshaft/hydrogen.cc | 9 ++++++- src/parser.cc | 22 --------------- src/parser.h | 1 - test/mjsunit/es6/classes.js | 27 ++++++++++++++++++- .../es6/debug-break-default-constructor.js | 12 ++++----- .../es6/debug-step-into-constructor.js | 11 ++++---- test/mjsunit/mjsunit.status | 2 -- 7 files changed, 45 insertions(+), 39 deletions(-) diff --git a/src/crankshaft/hydrogen.cc b/src/crankshaft/hydrogen.cc index 4771b0b2e4..f878a605f6 100644 --- a/src/crankshaft/hydrogen.cc +++ b/src/crankshaft/hydrogen.cc @@ -8357,6 +8357,11 @@ bool HOptimizedGraphBuilder::TryInline(Handle target, CompilationInfo target_info(&parse_info); Handle target_shared(target->shared()); + + if (IsClassConstructor(target_shared->kind())) { + TraceInline(target, caller, "target is classConstructor"); + return false; + } if (target_shared->HasDebugInfo()) { TraceInline(target, caller, "target is being debugged"); return false; @@ -9353,6 +9358,7 @@ bool HOptimizedGraphBuilder::TryIndirectCall(Call* expr) { } +// f.apply(...) void HOptimizedGraphBuilder::BuildFunctionApply(Call* expr) { ZoneList* args = expr->arguments(); CHECK_ALIVE(VisitForValue(args->at(0))); @@ -9751,7 +9757,8 @@ void HOptimizedGraphBuilder::VisitCall(Call* expr) { Push(graph()->GetConstantUndefined()); CHECK_ALIVE(VisitExpressions(expr->arguments())); - if (expr->IsMonomorphic()) { + if (expr->IsMonomorphic() && + !IsClassConstructor(expr->target()->shared()->kind())) { Add(function, expr->target()); // Patch the global object on the stack by the expected receiver. diff --git a/src/parser.cc b/src/parser.cc index 37cd361709..185a8de85e 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -359,7 +359,6 @@ FunctionLiteral* Parser::DefaultConstructor(bool call_super, Scope* scope, kind, &function_factory); body = new (zone()) ZoneList(call_super ? 2 : 1, zone()); - AddAssertIsConstruct(body, pos); if (call_super) { // %_DefaultConstructorCallSuper(new.target, %GetPrototype()) ZoneList* args = @@ -4536,21 +4535,6 @@ void Parser::SkipLazyFunctionBody(int* materialized_literal_count, } -void Parser::AddAssertIsConstruct(ZoneList* body, int pos) { - ZoneList* arguments = - new (zone()) ZoneList(0, zone()); - CallRuntime* construct_check = factory()->NewCallRuntime( - Runtime::kInlineIsConstructCall, arguments, pos); - CallRuntime* non_callable_error = factory()->NewCallRuntime( - Runtime::kThrowConstructorNonCallableError, arguments, pos); - IfStatement* if_statement = factory()->NewIfStatement( - factory()->NewUnaryOperation(Token::NOT, construct_check, pos), - factory()->NewReturnStatement(non_callable_error, pos), - factory()->NewEmptyStatement(pos), pos); - body->Add(if_statement, zone()); -} - - Statement* Parser::BuildAssertIsCoercible(Variable* var) { // if (var === null || var === undefined) // throw /* type error kNonCoercible) */; @@ -4744,12 +4728,6 @@ ZoneList* Parser::ParseEagerFunctionBody( result->Add(NULL, zone()); } - // For concise constructors, check that they are constructed, - // not called. - if (IsClassConstructor(kind)) { - AddAssertIsConstruct(result, pos); - } - ZoneList* body = result; Scope* inner_scope = scope_; Block* inner_block = nullptr; diff --git a/src/parser.h b/src/parser.h index 62ff59bfee..5aa1fdd2f8 100644 --- a/src/parser.h +++ b/src/parser.h @@ -1154,7 +1154,6 @@ class Parser : public ParserBase { BreakableStatement* LookupBreakTarget(const AstRawString* label, bool* ok); IterationStatement* LookupContinueTarget(const AstRawString* label, bool* ok); - void AddAssertIsConstruct(ZoneList* body, int pos); Statement* BuildAssertIsCoercible(Variable* var); // Factory methods. diff --git a/test/mjsunit/es6/classes.js b/test/mjsunit/es6/classes.js index 54cd165156..ac10f0e033 100644 --- a/test/mjsunit/es6/classes.js +++ b/test/mjsunit/es6/classes.js @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Flags: --harmony-sloppy +// Flags: --harmony-sloppy --allow-natives-syntax (function TestBasics() { var C = class C {} @@ -625,6 +625,7 @@ function assertAccessorDescriptor(object, name) { assertTrue(new C(1) instanceof C); })(); + (function TestConstructorCall(){ var realmIndex = Realm.create(); var otherTypeError = Realm.eval(realmIndex, "TypeError"); @@ -651,6 +652,30 @@ function assertAccessorDescriptor(object, name) { assertThrows(function() { A.call() }, otherTypeError); })(); + +(function TestConstructorCallOptimized() { + class A { }; + + function invoke_constructor() { A() } + function call_constructor() { A.call() } + function apply_constructor() { A.apply() } + + for (var i=0; i<3; i++) { + assertThrows(invoke_constructor); + assertThrows(call_constructor); + assertThrows(apply_constructor); + } + // Make sure we still check for class constructors when calling optimized + // code. + %OptimizeFunctionOnNextCall(invoke_constructor); + assertThrows(invoke_constructor); + %OptimizeFunctionOnNextCall(call_constructor); + assertThrows(call_constructor); + %OptimizeFunctionOnNextCall(apply_constructor); + assertThrows(apply_constructor); +})(); + + (function TestDefaultConstructor() { var calls = 0; class Base { diff --git a/test/mjsunit/es6/debug-break-default-constructor.js b/test/mjsunit/es6/debug-break-default-constructor.js index f0783e2a56..a06c3b52de 100644 --- a/test/mjsunit/es6/debug-break-default-constructor.js +++ b/test/mjsunit/es6/debug-break-default-constructor.js @@ -24,19 +24,19 @@ function listener(event, execState, eventData, data) { } class Base { - constructor() { // 2. - var x = 1; // 3. - } // 4. + constructor() { + var x = 1; // 2. + } // 3. } -class Derived extends Base {} // 1. // 5. +class Derived extends Base {} // 1. // 4. Debug.setListener(listener); var bp = Debug.setBreakPoint(Derived, 0); new Derived(); -Debug.setListener(null); // 6. +Debug.setListener(null); // 5. assertNull(exception); -assertEquals(6, step_count); +assertEquals(5, step_count); diff --git a/test/mjsunit/es6/debug-step-into-constructor.js b/test/mjsunit/es6/debug-step-into-constructor.js index 66fc0b99d0..1e903256fd 100644 --- a/test/mjsunit/es6/debug-step-into-constructor.js +++ b/test/mjsunit/es6/debug-step-into-constructor.js @@ -14,7 +14,6 @@ function listener(event, execState, eventData, data) { if (!done) { execState.prepareStep(Debug.StepAction.StepInto); var s = execState.frame().sourceLineText(); - print(s); assertTrue(s.indexOf('// ' + stepCount + '.') !== -1); stepCount++; } @@ -41,7 +40,7 @@ class Derived extends Base {} var bp = Debug.setBreakPoint(Base, 0); new Base(); - assertEquals(5, stepCount); + assertEquals(1, stepCount); Debug.clearBreakPoint(bp); })(); @@ -53,7 +52,7 @@ class Derived extends Base {} var bp = Debug.setBreakPoint(Base, 0); new Derived(); - assertEquals(5, stepCount); + assertEquals(1, stepCount); Debug.clearBreakPoint(bp); })(); @@ -69,7 +68,7 @@ class Derived extends Base {} var bp = Debug.setBreakPoint(f, 0); f(); - assertEquals(5, stepCount); + assertEquals(1, stepCount); Debug.clearBreakPoint(bp); })(); @@ -87,7 +86,7 @@ class Derived extends Base {} var bp = Debug.setBreakPoint(f, 0); f(); - assertEquals(5, stepCount); + assertEquals(1, stepCount); Debug.clearBreakPoint(bp); })(); @@ -105,7 +104,7 @@ class Derived extends Base {} var bp = Debug.setBreakPoint(f, 0); f(); - assertEquals(5, stepCount); + assertEquals(1, stepCount); Debug.clearBreakPoint(bp); })(); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index 6e71e50bab..abb232703d 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -76,8 +76,6 @@ 'whitespaces': [PASS, NO_VARIANTS], 'compiler/osr-assert': [PASS, NO_VARIANTS], 'es6/string-fromcodepoint': [PASS, NO_VARIANTS], - # TODO(cbruni): enable once we fix classConstructor calls in optimized code - 'es6/classes': [PASS, NO_VARIANTS], 'regress/regress-2185-2': [PASS, NO_VARIANTS], 'regress/regress-2612': [PASS, NO_VARIANTS],