[ast] Remove internalization before AST rewriting

src/parsing/rewriter.cc

Index: src/parsing/rewriter.cc
diff --git a/src/parsing/rewriter.cc b/src/parsing/rewriter.cc
index a303428644427e09c1172c68f3bf3ce1866e1506..b6376d7a6b053a88054cba3df3893de676c5961a 100644
--- a/src/parsing/rewriter.cc
+++ b/src/parsing/rewriter.cc
@@ -15,8 +15,8 @@ namespace internal {
 
 class Processor final : public AstVisitor<Processor> {
  public:
-  Processor(Isolate* isolate, DeclarationScope* closure_scope, Variable* result,
-            AstValueFactory* ast_value_factory)
+  Processor(uintptr_t stack_limit, DeclarationScope* closure_scope,
+            Variable* result, AstValueFactory* ast_value_factory)
       : result_(result),
         result_assigned_(false),
         replacement_(nullptr),
@@ -26,7 +26,7 @@ class Processor final : public AstVisitor<Processor> {
         closure_scope_(closure_scope),
         factory_(ast_value_factory) {
     DCHECK_EQ(closure_scope, closure_scope->GetClosureScope());
-    InitializeAstVisitor(isolate);
+    InitializeAstVisitor(stack_limit);
   }
 
   Processor(Parser* parser, DeclarationScope* closure_scope, Variable* result,
@@ -355,25 +355,18 @@ DECLARATION_NODE_LIST(DEF_VISIT)
 // Assumes code has been parsed.  Mutates the AST, so the AST should not
 // continue to be used in the case of failure.
 bool Rewriter::Rewrite(ParseInfo* info) {
-  {
-    DisallowHeapAllocation no_allocation;
-    DisallowHandleAllocation no_handles;
-    DisallowHandleDereference no_deref;
-
-    FunctionLiteral* function = info->literal();
-    DCHECK_NOT_NULL(function);
-    Scope* scope = function->scope();
-    DCHECK_NOT_NULL(scope);
-    if (!scope->is_script_scope() && !scope->is_eval_scope()) return true;
-  }
+  DisallowHeapAllocation no_allocation;
+  DisallowHandleAllocation no_handles;
+  DisallowHandleDereference no_deref;
 
   RuntimeCallTimerScope runtimeTimer(
       info->isolate(), &RuntimeCallStats::CompileRewriteReturnResult);
+
   FunctionLiteral* function = info->literal();
   DCHECK_NOT_NULL(function);
   Scope* scope = function->scope();
   DCHECK_NOT_NULL(scope);
-  DCHECK(ThreadId::Current().Equals(info->isolate()->thread_id()));
+  if (!scope->is_script_scope() && !scope->is_eval_scope()) return true;
 
   DeclarationScope* closure_scope = scope->GetClosureScope();
 
@@ -381,12 +374,15 @@ bool Rewriter::Rewrite(ParseInfo* info) {
   if (!body->is_empty()) {
     Variable* result = closure_scope->NewTemporary(
         info->ast_value_factory()->dot_result_string());
-    // The name string must be internalized at this point.
-    info->ast_value_factory()->Internalize(info->isolate());
-    DCHECK(!result->name().is_null());
-    Processor processor(info->isolate(), closure_scope, result,
-                        info->ast_value_factory());
+    Processor processor(info->isolate()->stack_guard()->real_climit(),
+                        closure_scope, result, info->ast_value_factory());

[rmcilroy, 2017/01/16 14:54:59]

It looks like the Processor is only using ast_value_factory for constant
strings. After https://codereview.chromium.org/2630343002/ lands could this just
take the AstStringConstants instead?

[Leszek Swirski, 2017/01/16 15:21:13]

Certainly, do you want me to rebase on top of that and make this patch
dependent?

[rmcilroy, 2017/01/16 15:35:11]

Yeah that would be good. That CL should land today anyway.

     processor.Process(body);
+
+    DCHECK(ThreadId::Current().Equals(info->isolate()->thread_id()));

[rmcilroy, 2017/01/17 10:41:32]

nit - please add a TODO to remove these releases once internalization is moved
to the finalization stage.

[Leszek Swirski, 2017/01/17 12:11:49]

Done.

+    no_deref.Release();
+    no_handles.Release();
+    no_allocation.Release();
+
     // Internalize any values created during rewriting.
     info->ast_value_factory()->Internalize(info->isolate());

[rmcilroy, 2017/01/16 14:54:59]

If we use AstStringConstants above the values will already be internalized, so
this internalize here should no longer be necessary, at which point the whole
class might be off-thread capable :).

     if (processor.HasStackOverflow()) return false;