Revert of Preparse inner functions (new try) (patchset #21 id:420001 of...

Revert of Preparse inner functions (new try) (patchset #21 id:420001 of https://codereview.chromium.org/2352593002/ )

Reason for revert:
We currently have some stability issues on Canary. Let's reland this after we verified that we "fixed" Canary again.

Original issue's description:
> Preparse inner functions (new try)
>
> This is an overly pessimistic approach where PreParser only keeps
> track of unresolved variables, but doesn't declare anything. This
> will result in context-allocating variables in the outer function
> unnecessarily, if the variable names clash with variable names
> used by the inner function (even if the variables are not the
> same). However, we have been unable to prove that this approach
> wouldn't be good enough for the practical purposes.
>
> Fixes after the previous try ( https://codereview.chromium.org/2322243002/ ):
> Keep the context-allocation decision stable when compiling fully eagerly.
>
> Tests which exercise this functionality:
> mjsunit/fixed-context-shapes-when-recompiling.js
>
> Design document (chromium):
>
> https://docs.google.com/a/chromium.org/document/d/1rRv5JJZ0JpOZAZN2CSUwZPFJiBAdRnTiSYhazseNHFg/edit?usp=sharing
>
> BUG=
>
> Committed: https://crrev.com/7c73cf32c60484cdf37c84f1d61b4640e87068d7
> Cr-Commit-Position: refs/heads/master@{#39719}

TBR=verwaest@chromium.org,adamk@chromium.org,marja@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=

Review-Url: https://codereview.chromium.org/2373443003
Cr-Commit-Position: refs/heads/master@{#39730}

src/ast/scopes.cc

@@ -1073,14 +1073,12 @@ void DeclarationScope::AllocateVariables(ParseInfo* info, AnalyzeMode mode) {
   }
 }
 
-bool Scope::AllowsLazyParsingWithoutUnresolvedVariables() const {
-  // If we are inside a block scope, we must find unresolved variables in the
-  // inner scopes to find out how to allocate variables on the block scope. At
-  // this point, declarations may not have yet been parsed.
+bool Scope::AllowsLazyParsing() const {
+  // If we are inside a block scope, we must parse eagerly to find out how
+  // to allocate variables on the block scope. At this point, declarations may
+  // not have yet been parsed.
   for (const Scope* s = this; s != nullptr; s = s->outer_scope_) {
     if (s->is_block_scope()) return false;
-    // TODO(marja): Refactor parsing modes: also add s->is_function_scope()
-    // here.
   }
   return true;
 }
@@ -1180,7 +1178,7 @@ Scope* Scope::GetOuterScopeWithContext() {
 
 Handle<StringSet> DeclarationScope::CollectNonLocals(
     ParseInfo* info, Handle<StringSet> non_locals) {
-  VariableProxy* free_variables = FetchFreeVariables(this, true, info);
+  VariableProxy* free_variables = FetchFreeVariables(this, info);
   for (VariableProxy* proxy = free_variables; proxy != nullptr;
        proxy = proxy->next_unresolved()) {
     non_locals = StringSet::Add(non_locals, proxy->name());
@@ -1193,9 +1191,8 @@ void DeclarationScope::AnalyzePartially(DeclarationScope* migrate_to,
   // Try to resolve unresolved variables for this Scope and migrate those which
   // cannot be resolved inside. It doesn't make sense to try to resolve them in
   // the outer Scopes here, because they are incomplete.
-  for (VariableProxy* proxy =
-           FetchFreeVariables(this, !FLAG_lazy_inner_functions);
-       proxy != nullptr; proxy = proxy->next_unresolved()) {
+  for (VariableProxy* proxy = FetchFreeVariables(this); proxy != nullptr;
+       proxy = proxy->next_unresolved()) {
     DCHECK(!proxy->is_resolved());
     VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
     migrate_to->AddUnresolved(copy);
@@ -1518,29 +1515,6 @@ void Scope::ResolveVariable(ParseInfo* info, VariableProxy* proxy) {
   DCHECK(!proxy->is_resolved());
   Variable* var = LookupRecursive(proxy, nullptr);
   ResolveTo(info, proxy, var);
-
-  if (FLAG_lazy_inner_functions) {
-    if (info != nullptr && info->is_native()) return;
-    // Pessimistically force context allocation for all variables to which inner
-    // scope variables could potentially resolve to.
-    Scope* scope = GetClosureScope()->outer_scope_;
-    while (scope != nullptr && scope->scope_info_.is_null()) {
-      var = scope->LookupLocal(proxy->raw_name());
-      if (var != nullptr) {
-        // Since we don't lazy parse inner arrow functions, inner functions
-        // cannot refer to the outer "this".
-        if (!var->is_dynamic() && !var->is_this() &&
-            !var->has_forced_context_allocation()) {
-          var->ForceContextAllocation();
-          var->set_is_used();
-          // We don't know what the (potentially lazy parsed) inner function
-          // does with the variable; pessimistically assume that it's assigned.
-          var->set_maybe_assigned();
-        }
-      }
-      scope = scope->outer_scope_;
-    }
-  }
 }
 
 void Scope::ResolveTo(ParseInfo* info, VariableProxy* proxy, Variable* var) {
@@ -1586,16 +1560,13 @@ void Scope::ResolveVariablesRecursively(ParseInfo* info) {
 }
 
 VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
-                                         bool try_to_resolve, ParseInfo* info,
+                                         ParseInfo* info,
                                          VariableProxy* stack) {
   for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
        proxy = next) {
     next = proxy->next_unresolved();
     DCHECK(!proxy->is_resolved());
-    Variable* var = nullptr;
-    if (try_to_resolve) {
-      var = LookupRecursive(proxy, max_outer_scope->outer_scope());
-    }
+    Variable* var = LookupRecursive(proxy, max_outer_scope->outer_scope());
     if (var == nullptr) {
       proxy->set_next_unresolved(stack);
       stack = proxy;
@@ -1608,8 +1579,7 @@ VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
   unresolved_ = nullptr;
 
   for (Scope* scope = inner_scope_; scope != nullptr; scope = scope->sibling_) {
-    stack =
-        scope->FetchFreeVariables(max_outer_scope, try_to_resolve, info, stack);
+    stack = scope->FetchFreeVariables(max_outer_scope, info, stack);
   }
 
   return stack;

src/ast/scopes.h

@@ -342,9 +342,8 @@ class Scope: public ZoneObject {
   int StackLocalCount() const;
   int ContextLocalCount() const;
 
-  // Determine if we can parse a function literal in this scope lazily without
-  // caring about the unresolved variables within.
-  bool AllowsLazyParsingWithoutUnresolvedVariables() const;
+  // Determine if we can parse a function literal in this scope lazily.
+  bool AllowsLazyParsing() const;
 
   // The number of contexts between this and scope; zero if this == scope.
   int ContextChainLength(Scope* scope) const;
@@ -539,7 +538,6 @@ class Scope: public ZoneObject {
   // list along the way, so full resolution cannot be done afterwards.
   // If a ParseInfo* is passed, non-free variables will be resolved.
   VariableProxy* FetchFreeVariables(DeclarationScope* max_outer_scope,
-                                    bool try_to_resolve = true,
                                     ParseInfo* info = nullptr,
                                     VariableProxy* stack = nullptr);
 

src/flag-definitions.h

@@ -837,7 +837,6 @@ DEFINE_BOOL(trace_maps, false, "trace map creation")
 // parser.cc
 DEFINE_BOOL(allow_natives_syntax, false, "allow natives syntax")
 DEFINE_BOOL(trace_parse, false, "trace parsing and preparsing")
-DEFINE_BOOL(lazy_inner_functions, true, "enable lazy parsing inner functions")
 
 // simulator-arm.cc, simulator-arm64.cc and simulator-mips.cc
 DEFINE_BOOL(trace_sim, false, "Trace simulator execution")

src/parsing/parser-base.h

@@ -3915,17 +3915,13 @@ ParserBase<Impl>::ParseArrowFunctionLiteral(
       // Multiple statement body
       Consume(Token::LBRACE);
       DCHECK_EQ(scope(), formal_parameters.scope);
-      bool is_lazily_parsed =
-          (mode() == PARSE_LAZILY &&
-           formal_parameters.scope
-               ->AllowsLazyParsingWithoutUnresolvedVariables());
-      // TODO(marja): consider lazy-parsing inner arrow functions too. is_this
-      // handling in Scope::ResolveVariable needs to change.
+      bool is_lazily_parsed = (mode() == PARSE_LAZILY &&
+                               formal_parameters.scope->AllowsLazyParsing());
       if (is_lazily_parsed) {
         Scanner::BookmarkScope bookmark(scanner());
         bookmark.Set();
         LazyParsingResult result = impl()->SkipLazyFunctionBody(
-            &materialized_literal_count, &expected_property_count, false, true,
+            &materialized_literal_count, &expected_property_count, true,
             CHECK_OK);
 
         if (formal_parameters.materialized_literals_count > 0) {

src/parsing/parser.h

@@ -484,11 +484,10 @@ class Parser : public ParserBase<Parser> {
   // in order to force the function to be eagerly parsed, after all.
   LazyParsingResult SkipLazyFunctionBody(int* materialized_literal_count,
                                          int* expected_property_count,
-                                         bool is_inner_function, bool may_abort,
-                                         bool* ok);
+                                         bool may_abort, bool* ok);
 
   PreParser::PreParseResult ParseLazyFunctionBodyWithPreParser(
-      SingletonLogger* logger, bool is_inner_function, bool may_abort);
+      SingletonLogger* logger, bool may_abort);
 
   Block* BuildParameterInitializationBlock(
       const ParserFormalParameters& parameters, bool* ok);
@@ -1064,9 +1063,6 @@ class Parser : public ParserBase<Parser> {
   }
 
   // Parser's private field members.
-  friend class DiscardableZoneScope;  // Uses reusable_preparser_.
-  // FIXME(marja): Make reusable_preparser_ always use its own temp Zone (call
-  // DeleteAll after each function), so this won't be needed.
 
   Scanner scanner_;
   PreParser* reusable_preparser_;

src/parsing/preparser.cc

@@ -38,10 +38,8 @@ namespace internal {
 #define CHECK_OK CHECK_OK_VALUE(Statement::Default())
 #define CHECK_OK_VOID CHECK_OK_VALUE(this->Void())
 
-namespace {
-
-PreParserIdentifier GetSymbolHelper(Scanner* scanner) {
-  switch (scanner->current_token()) {
+PreParserIdentifier PreParser::GetSymbol() const {
+  switch (scanner()->current_token()) {
     case Token::ENUM:
       return PreParserIdentifier::Enum();
     case Token::AWAIT:
@@ -57,46 +55,34 @@ PreParserIdentifier GetSymbolHelper(Scanner* scanner) {
     case Token::ASYNC:
       return PreParserIdentifier::Async();
     default:
-      if (scanner->UnescapedLiteralMatches("eval", 4))
+      if (scanner()->UnescapedLiteralMatches("eval", 4))
         return PreParserIdentifier::Eval();
-      if (scanner->UnescapedLiteralMatches("arguments", 9))
+      if (scanner()->UnescapedLiteralMatches("arguments", 9))
         return PreParserIdentifier::Arguments();
-      if (scanner->UnescapedLiteralMatches("undefined", 9))
+      if (scanner()->UnescapedLiteralMatches("undefined", 9))
         return PreParserIdentifier::Undefined();
-      if (scanner->LiteralMatches("prototype", 9))
+      if (scanner()->LiteralMatches("prototype", 9))
         return PreParserIdentifier::Prototype();
-      if (scanner->LiteralMatches("constructor", 11))
+      if (scanner()->LiteralMatches("constructor", 11))
         return PreParserIdentifier::Constructor();
       return PreParserIdentifier::Default();
   }
 }
 
-}  // unnamed namespace
-
-PreParserIdentifier PreParser::GetSymbol() const {
-  PreParserIdentifier symbol = GetSymbolHelper(scanner());
-  if (track_unresolved_variables_) {
-    const AstRawString* result = scanner()->CurrentSymbol(ast_value_factory());
-    DCHECK_NOT_NULL(result);
-    symbol.string_ = result;
-  }
-  return symbol;
-}
-
 PreParser::PreParseResult PreParser::PreParseLazyFunction(
-    FunctionKind kind, DeclarationScope* function_scope, bool parsing_module,
-    ParserRecorder* log, bool is_inner_function, bool may_abort,
-    int* use_counts) {
+    LanguageMode language_mode, FunctionKind kind, bool has_simple_parameters,
+    bool parsing_module, ParserRecorder* log, bool may_abort, int* use_counts) {
   parsing_module_ = parsing_module;
   log_ = log;
   use_counts_ = use_counts;
-  DCHECK(!track_unresolved_variables_);
-  track_unresolved_variables_ = is_inner_function;
-
-  // The caller passes the function_scope which is not yet inserted into the
-  // scope_state_. All scopes above the function_scope are ignored by the
-  // PreParser.
+  // Lazy functions always have trivial outer scopes (no with/catch scopes).
   DCHECK_NULL(scope_state_);
+  DeclarationScope* top_scope = NewScriptScope();
+  FunctionState top_state(&function_state_, &scope_state_, top_scope,
+                          kNormalFunction);
+  scope()->SetLanguageMode(language_mode);
+  DeclarationScope* function_scope = NewFunctionScope(kind);
+  if (!has_simple_parameters) function_scope->SetHasNonSimpleParameters();
   FunctionState function_state(&function_state_, &scope_state_, function_scope,
                                kind);
   DCHECK_EQ(Token::LBRACE, scanner()->current_token());
@@ -104,7 +90,6 @@ PreParser::PreParseResult PreParser::PreParseLazyFunction(
   int start_position = peek_position();
   LazyParsingResult result = ParseLazyFunctionLiteralBody(may_abort, &ok);
   use_counts_ = nullptr;
-  track_unresolved_variables_ = false;
   if (result == kLazyParsingAborted) {
     return kPreParseAbort;
   } else if (stack_overflow()) {
@@ -355,21 +340,6 @@ void PreParser::ParseAsyncArrowSingleExpressionBody(PreParserStatementList body,
   body->Add(PreParserStatement::ExpressionStatement(return_value), zone());
 }
 
-PreParserExpression PreParser::ExpressionFromIdentifier(
-    PreParserIdentifier name, int start_position, int end_position,
-    InferName infer) {
-  if (track_unresolved_variables_) {
-    AstNodeFactory factory(ast_value_factory());
-    // Setting the Zone is necessary because zone_ might be the temp Zone, and
-    // AstValueFactory doesn't know about it.
-    factory.set_zone(zone());
-    DCHECK_NOT_NULL(name.string_);
-    scope()->NewUnresolved(&factory, name.string_, start_position, end_position,
-                           NORMAL_VARIABLE);
-  }
-  return PreParserExpression::FromIdentifier(name);
-}
-
 #undef CHECK_OK
 #undef CHECK_OK_CUSTOM
 

src/parsing/preparser.h

@@ -112,12 +112,10 @@ class PreParserIdentifier {
     kAsyncIdentifier
   };
 
-  explicit PreParserIdentifier(Type type) : type_(type), string_(nullptr) {}
+  explicit PreParserIdentifier(Type type) : type_(type) {}
   Type type_;
-  // Only non-nullptr when PreParser.track_unresolved_variables_ is true.
-  const AstRawString* string_;
+
   friend class PreParserExpression;
-  friend class PreParser;
 };
 
 
@@ -776,8 +774,7 @@ class PreParser : public ParserBase<PreParser> {
             ParserRecorder* log, uintptr_t stack_limit)
       : ParserBase<PreParser>(zone, scanner, stack_limit, NULL,
                               ast_value_factory, log),
-        use_counts_(nullptr),
-        track_unresolved_variables_(false) {}
+        use_counts_(nullptr) {}
 
   // Pre-parse the program from the character stream; returns true on
   // success (even if parsing failed, the pre-parse data successfully
@@ -823,10 +820,10 @@ class PreParser : public ParserBase<PreParser> {
   // keyword and parameters, and have consumed the initial '{'.
   // At return, unless an error occurred, the scanner is positioned before the
   // the final '}'.
-  PreParseResult PreParseLazyFunction(FunctionKind kind,
-                                      DeclarationScope* function_scope,
+  PreParseResult PreParseLazyFunction(LanguageMode language_mode,
+                                      FunctionKind kind,
+                                      bool has_simple_parameters,
                                       bool parsing_module, ParserRecorder* log,
-                                      bool track_unresolved_variables,
                                       bool may_abort, int* use_counts);
 
  private:
@@ -853,9 +850,9 @@ class PreParser : public ParserBase<PreParser> {
       const PreParserFormalParameters& parameters, FunctionKind kind,
       FunctionLiteral::FunctionType function_type, bool* ok);
 
-  V8_INLINE LazyParsingResult SkipLazyFunctionBody(
-      int* materialized_literal_count, int* expected_property_count,
-      bool track_unresolved_variables, bool may_abort, bool* ok) {
+  V8_INLINE LazyParsingResult
+  SkipLazyFunctionBody(int* materialized_literal_count,
+                       int* expected_property_count, bool may_abort, bool* ok) {
     UNREACHABLE();
     return kLazyParsingComplete;
   }
@@ -1307,9 +1304,11 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserExpression::Default();
   }
 
-  PreParserExpression ExpressionFromIdentifier(
+  V8_INLINE PreParserExpression ExpressionFromIdentifier(
       PreParserIdentifier name, int start_position, int end_position,
-      InferName infer = InferName::kYes);
+      InferName infer = InferName::kYes) {
+    return PreParserExpression::FromIdentifier(name);
+  }
 
   V8_INLINE PreParserExpression ExpressionFromString(int pos) {
     if (scanner()->UnescapedLiteralMatches("use strict", 10)) {
@@ -1418,7 +1417,6 @@ class PreParser : public ParserBase<PreParser> {
   // Preparser's private field members.
 
   int* use_counts_;
-  bool track_unresolved_variables_;
 };
 
 PreParserExpression PreParser::SpreadCall(PreParserExpression function,

test/cctest/test-parsing.cc

@@ -38,7 +38,6 @@
 #include "src/ast/ast.h"
 #include "src/compiler.h"
 #include "src/execution.h"
-#include "src/flags.h"
 #include "src/isolate.h"
 #include "src/objects.h"
 #include "src/parsing/parse-info.h"
@@ -3417,14 +3416,7 @@ TEST(InnerAssignment) {
         bool expected = outers[i].assigned || inners[j].assigned;
         CHECK(var != NULL);
         CHECK(var->is_used() || !expected);
-        bool is_maybe_assigned = var->maybe_assigned() == i::kMaybeAssigned;
-        if (i::FLAG_lazy_inner_functions) {
-          // If we parse inner functions lazily, allow being pessimistic about
-          // maybe_assigned.
-          CHECK(is_maybe_assigned || (is_maybe_assigned == expected));
-        } else {
-          CHECK(is_maybe_assigned == expected);
-        }
+        CHECK((var->maybe_assigned() == i::kMaybeAssigned) == expected);
       }
     }
   }

test/mjsunit/debug-function-scopes.js

@@ -87,6 +87,7 @@ var f3 = (function F1(invisible_parameter) {
     var invisible2 = 2;
     return (function F3() {
       var visible2 = 20;
+      var invisible2 = 3;
       return (function () {return visible1 + visible2 + visible1a;});
     })();
   })();

test/mjsunit/debug-scopes.js

@@ -157,20 +157,20 @@ function CheckScopeChainNames(names, exec_state) {
 }
 
 
-// Check that the scope contains at least minimum_content. For functions just
-// check that there is a function.
-function CheckScopeContent(minimum_content, number, exec_state) {
+// Check that the content of the scope is as expected. For functions just check
+// that there is a function.
+function CheckScopeContent(content, number, exec_state) {
   var scope = exec_state.frame().scope(number);
-  var minimum_count = 0;
-  for (var p in minimum_content) {
+  var count = 0;
+  for (var p in content) {
     var property_mirror = scope.scopeObject().property(p);
     assertFalse(property_mirror.isUndefined(), 'property ' + p + ' not found in scope');
-    if (typeof(minimum_content[p]) === 'function') {
+    if (typeof(content[p]) === 'function') {
       assertTrue(property_mirror.value().isFunction());
     } else {
-      assertEquals(minimum_content[p], property_mirror.value().value(), 'property ' + p + ' has unexpected value');
+      assertEquals(content[p], property_mirror.value().value(), 'property ' + p + ' has unexpected value');
     }
-    minimum_count++;
+    count++;
   }
 
   // 'arguments' and might be exposed in the local and closure scope. Just
@@ -186,14 +186,14 @@ function CheckScopeContent(minimum_content, number, exec_state) {
   // Temporary variables introduced by the parser have not been materialized.
   assertTrue(scope.scopeObject().property('').isUndefined());
 
-  if (scope_size < minimum_count) {
+  if (count != scope_size) {
     print('Names found in scope:');
     var names = scope.scopeObject().propertyNames();
     for (var i = 0; i < names.length; i++) {
       print(names[i]);
     }
   }
-  assertTrue(scope_size >= minimum_count);
+  assertEquals(count, scope_size);
 
   // Get the debug command processor.
   var dcp = exec_state.debugCommandProcessor("unspecified_running_state");

test/mjsunit/lazy-inner-functions.js

-// Copyright 2016 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.
-
-// Flags: --min-preparse-length 1
-
-(function TestLazyInnerFunctionCallsEval() {
-  var i = (function eager_outer() {
-    var a = 41; // Should be context-allocated
-    function lazy_inner() {
-      return eval("a");
-    }
-    return lazy_inner;
-  })();
-  assertEquals(41, i());
-})();