Reland of Preparse inner functions (new try) (patchset #1 id:1 of...

Reland of Preparse inner functions (new try) (patchset #1 id:1 of https://codereview.chromium.org/2373443003/ )

Reason for revert:
Stability thief found, relanding speculative reverts.

Original issue's description:
> 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=
>
> Committed: https://crrev.com/1e6296b2a7cfc307fd9e722e619f42965da4a267
> Cr-Commit-Position: refs/heads/master@{#39730}

TBR=verwaest@chromium.org,adamk@chromium.org,marja@chromium.org,hablich@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/2377513006
Cr-Commit-Position: refs/heads/master@{#39755}

src/ast/scopes.cc

@@ -1073,12 +1073,14 @@ void DeclarationScope::AllocateVariables(ParseInfo* info, AnalyzeMode mode) {
   }
 }
 
-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.
+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.
   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;
 }
@@ -1178,7 +1180,7 @@ Scope* Scope::GetOuterScopeWithContext() {
 
 Handle<StringSet> DeclarationScope::CollectNonLocals(
     ParseInfo* info, Handle<StringSet> non_locals) {
-  VariableProxy* free_variables = FetchFreeVariables(this, info);
+  VariableProxy* free_variables = FetchFreeVariables(this, true, info);
   for (VariableProxy* proxy = free_variables; proxy != nullptr;
        proxy = proxy->next_unresolved()) {
     non_locals = StringSet::Add(non_locals, proxy->name());
@@ -1191,8 +1193,9 @@ 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); proxy != nullptr;
-       proxy = proxy->next_unresolved()) {
+  for (VariableProxy* proxy =
+           FetchFreeVariables(this, !FLAG_lazy_inner_functions);
+       proxy != nullptr; proxy = proxy->next_unresolved()) {
     DCHECK(!proxy->is_resolved());
     VariableProxy* copy = ast_node_factory->CopyVariableProxy(proxy);
     migrate_to->AddUnresolved(copy);
@@ -1515,6 +1518,29 @@ 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) {
@@ -1560,13 +1586,16 @@ void Scope::ResolveVariablesRecursively(ParseInfo* info) {
 }
 
 VariableProxy* Scope::FetchFreeVariables(DeclarationScope* max_outer_scope,
-                                         ParseInfo* info,
+                                         bool try_to_resolve, ParseInfo* info,
                                          VariableProxy* stack) {
   for (VariableProxy *proxy = unresolved_, *next = nullptr; proxy != nullptr;
        proxy = next) {
     next = proxy->next_unresolved();
     DCHECK(!proxy->is_resolved());
-    Variable* var = LookupRecursive(proxy, max_outer_scope->outer_scope());
+    Variable* var = nullptr;
+    if (try_to_resolve) {
+      var = LookupRecursive(proxy, max_outer_scope->outer_scope());
+    }
     if (var == nullptr) {
       proxy->set_next_unresolved(stack);
       stack = proxy;
@@ -1579,7 +1608,8 @@ 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, info, stack);
+    stack =
+        scope->FetchFreeVariables(max_outer_scope, try_to_resolve, info, stack);
   }
 
   return stack;

src/ast/scopes.h

@@ -342,8 +342,9 @@ class Scope: public ZoneObject {
   int StackLocalCount() const;
   int ContextLocalCount() const;
 
-  // Determine if we can parse a function literal in this scope lazily.
-  bool AllowsLazyParsing() const;
+  // Determine if we can parse a function literal in this scope lazily without
+  // caring about the unresolved variables within.
+  bool AllowsLazyParsingWithoutUnresolvedVariables() const;
 
   // The number of contexts between this and scope; zero if this == scope.
   int ContextChainLength(Scope* scope) const;
@@ -538,6 +539,7 @@ 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

@@ -838,6 +838,7 @@ 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,13 +3915,17 @@ 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->AllowsLazyParsing());
+      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.
       if (is_lazily_parsed) {
         Scanner::BookmarkScope bookmark(scanner());
         bookmark.Set();
         LazyParsingResult result = impl()->SkipLazyFunctionBody(
-            &materialized_literal_count, &expected_property_count, true,
+            &materialized_literal_count, &expected_property_count, false, true,
             CHECK_OK);
 
         if (formal_parameters.materialized_literals_count > 0) {

src/parsing/parser.h

@@ -484,10 +484,11 @@ 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 may_abort, bool* ok);
+                                         bool is_inner_function, bool may_abort,
+                                         bool* ok);
 
   PreParser::PreParseResult ParseLazyFunctionBodyWithPreParser(
-      SingletonLogger* logger, bool may_abort);
+      SingletonLogger* logger, bool is_inner_function, bool may_abort);
 
   Block* BuildParameterInitializationBlock(
       const ParserFormalParameters& parameters, bool* ok);
@@ -1063,6 +1064,9 @@ 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,8 +38,10 @@ namespace internal {
 #define CHECK_OK CHECK_OK_VALUE(Statement::Default())
 #define CHECK_OK_VOID CHECK_OK_VALUE(this->Void())
 
-PreParserIdentifier PreParser::GetSymbol() const {
-  switch (scanner()->current_token()) {
+namespace {
+
+PreParserIdentifier GetSymbolHelper(Scanner* scanner) {
+  switch (scanner->current_token()) {
     case Token::ENUM:
       return PreParserIdentifier::Enum();
     case Token::AWAIT:
@@ -55,34 +57,46 @@ PreParserIdentifier PreParser::GetSymbol() const {
     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(
-    LanguageMode language_mode, FunctionKind kind, bool has_simple_parameters,
-    bool parsing_module, ParserRecorder* log, bool may_abort, int* use_counts) {
+    FunctionKind kind, DeclarationScope* function_scope, bool parsing_module,
+    ParserRecorder* log, bool is_inner_function, bool may_abort,
+    int* use_counts) {
   parsing_module_ = parsing_module;
   log_ = log;
   use_counts_ = use_counts;
-  // Lazy functions always have trivial outer scopes (no with/catch scopes).
+  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.
   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());
@@ -90,6 +104,7 @@ 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()) {
@@ -340,6 +355,21 @@ 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,10 +112,12 @@ class PreParserIdentifier {
     kAsyncIdentifier
   };
 
-  explicit PreParserIdentifier(Type type) : type_(type) {}
+  explicit PreParserIdentifier(Type type) : type_(type), string_(nullptr) {}
   Type type_;
-
+  // Only non-nullptr when PreParser.track_unresolved_variables_ is true.
+  const AstRawString* string_;
   friend class PreParserExpression;
+  friend class PreParser;
 };
 
 
@@ -774,7 +776,8 @@ 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) {}
+        use_counts_(nullptr),
+        track_unresolved_variables_(false) {}
 
   // Pre-parse the program from the character stream; returns true on
   // success (even if parsing failed, the pre-parse data successfully
@@ -820,10 +823,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(LanguageMode language_mode,
-                                      FunctionKind kind,
-                                      bool has_simple_parameters,
+  PreParseResult PreParseLazyFunction(FunctionKind kind,
+                                      DeclarationScope* function_scope,
                                       bool parsing_module, ParserRecorder* log,
+                                      bool track_unresolved_variables,
                                       bool may_abort, int* use_counts);
 
  private:
@@ -850,9 +853,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 may_abort, bool* ok) {
+  V8_INLINE LazyParsingResult SkipLazyFunctionBody(
+      int* materialized_literal_count, int* expected_property_count,
+      bool track_unresolved_variables, bool may_abort, bool* ok) {
     UNREACHABLE();
     return kLazyParsingComplete;
   }
@@ -1304,11 +1307,9 @@ class PreParser : public ParserBase<PreParser> {
     return PreParserExpression::Default();
   }
 
-  V8_INLINE PreParserExpression ExpressionFromIdentifier(
+  PreParserExpression ExpressionFromIdentifier(
       PreParserIdentifier name, int start_position, int end_position,
-      InferName infer = InferName::kYes) {
-    return PreParserExpression::FromIdentifier(name);
-  }
+      InferName infer = InferName::kYes);
 
   V8_INLINE PreParserExpression ExpressionFromString(int pos) {
     if (scanner()->UnescapedLiteralMatches("use strict", 10)) {
@@ -1417,6 +1418,7 @@ 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,6 +38,7 @@
 #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"
@@ -3416,7 +3417,14 @@ TEST(InnerAssignment) {
         bool expected = outers[i].assigned || inners[j].assigned;
         CHECK(var != NULL);
         CHECK(var->is_used() || !expected);
-        CHECK((var->maybe_assigned() == i::kMaybeAssigned) == 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);
+        }
       }
     }
   }

test/mjsunit/debug-function-scopes.js

@@ -87,7 +87,6 @@ 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 content of the scope is as expected. For functions just check
-// that there is a function.
-function CheckScopeContent(content, number, 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) {
   var scope = exec_state.frame().scope(number);
-  var count = 0;
-  for (var p in content) {
+  var minimum_count = 0;
+  for (var p in minimum_content) {
     var property_mirror = scope.scopeObject().property(p);
     assertFalse(property_mirror.isUndefined(), 'property ' + p + ' not found in scope');
-    if (typeof(content[p]) === 'function') {
+    if (typeof(minimum_content[p]) === 'function') {
       assertTrue(property_mirror.value().isFunction());
     } else {
-      assertEquals(content[p], property_mirror.value().value(), 'property ' + p + ' has unexpected value');
+      assertEquals(minimum_content[p], property_mirror.value().value(), 'property ' + p + ' has unexpected value');
     }
-    count++;
+    minimum_count++;
   }
 
   // 'arguments' and might be exposed in the local and closure scope. Just
@@ -186,14 +186,14 @@ function CheckScopeContent(content, number, exec_state) {
   // Temporary variables introduced by the parser have not been materialized.
   assertTrue(scope.scopeObject().property('').isUndefined());
 
-  if (count != scope_size) {
+  if (scope_size < minimum_count) {
     print('Names found in scope:');
     var names = scope.scopeObject().propertyNames();
     for (var i = 0; i < names.length; i++) {
       print(names[i]);
     }
   }
-  assertEquals(count, scope_size);
+  assertTrue(scope_size >= minimum_count);
 
   // 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());
+})();