Revert of [Parser] Introduce AstStringConstants to share constants across...

Revert of [Parser] Introduce AstStringConstants to share constants across AstValueFactory (patchset #4 id:80001 of https://codereview.chromium.org/2630343002/ )

Reason for revert:
Seems to break modules-namespace2 on gcstress.

Original issue's description:
> [Parser] Introduce AstStringConstants to share constants across AstValueFactory
>
> Creates an AstStringConstants container which pre-initializes the
> string constants used by AstValueFactory. This ensures that all
> AstValueFactories will produce the same AstValue objects for constants,
> and so they can be used by the BytecodeGenerator without having to pass
> the AstValueFactory to it, enabling construction off-thread.
>
> BUG=v8:5203
>
> Review-Url: https://codereview.chromium.org/2630343002
> Cr-Commit-Position: refs/heads/master@{#42381}
> Committed: https://chromium.googlesource.com/v8/v8/+/d611496b8ed30af787d8668f96b400617c858508

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

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

src/ast/ast-value-factory.cc

@@ -221,6 +221,7 @@ void AstValue::Internalize(Isolate* isolate) {
   }
 }
 
+
 AstRawString* AstValueFactory::GetOneByteStringInternal(
     Vector<const uint8_t> literal) {
   if (literal.length() == 1 && IsInRange(literal[0], 'a', 'z')) {

src/ast/ast-value-factory.h

@@ -111,9 +111,8 @@ class AstRawString final : public AstString {
   }
 
  private:
-  friend class AstRawStringInternalizationKey;
-  friend class AstStringConstants;
   friend class AstValueFactory;
+  friend class AstRawStringInternalizationKey;
 
   AstRawString(bool is_one_byte, const Vector<const byte>& literal_bytes,
                uint32_t hash)
@@ -293,6 +292,7 @@ class AstValue : public ZoneObject {
   };
 };
 
+
 // For generating constants.
 #define STRING_CONSTANTS(F)                     \
   F(anonymous_function, "(anonymous function)") \
@@ -332,45 +332,6 @@ class AstValue : public ZoneObject {
   F(use_strict, "use strict")                   \
   F(value, "value")
 
-class AstStringConstants final {
- public:
-  AstStringConstants(Isolate* isolate, uint32_t hash_seed)
-      : zone_(isolate->allocator(), ZONE_NAME), hash_seed_(hash_seed) {
-    DCHECK(ThreadId::Current().Equals(isolate->thread_id()));
-#define F(name, str)                                                      \
-  {                                                                       \
-    const char* data = str;                                               \
-    Vector<const uint8_t> literal(reinterpret_cast<const uint8_t*>(data), \
-                                  static_cast<int>(strlen(data)));        \
-    uint32_t hash = StringHasher::HashSequentialString<uint8_t>(          \
-        literal.start(), literal.length(), hash_seed_);                   \
-    name##_string_ = new (&zone_) AstRawString(true, literal, hash);      \
-    /* The Handle returned by the factory is located on the roots */      \
-    /* array, not on the temporary HandleScope, so this is safe.  */      \
-    name##_string_->set_string(isolate->factory()->name##_string());      \
-  }
-    STRING_CONSTANTS(F)
-#undef F
-  }
-
-#define F(name, str) \
-  AstRawString* name##_string() { return name##_string_; }
-  STRING_CONSTANTS(F)
-#undef F
-
-  uint32_t hash_seed() const { return hash_seed_; }
-
- private:
-  Zone zone_;
-  uint32_t hash_seed_;
-
-#define F(name, str) AstRawString* name##_string_;
-  STRING_CONSTANTS(F)
-#undef F
-
-  DISALLOW_COPY_AND_ASSIGN(AstStringConstants);
-};
-
 #define OTHER_CONSTANTS(F) \
   F(true_value)            \
   F(false_value)           \
@@ -380,24 +341,23 @@ class AstStringConstants final {
 
 class AstValueFactory {
  public:
-  AstValueFactory(Zone* zone, AstStringConstants* string_constants,
-                  uint32_t hash_seed)
+  AstValueFactory(Zone* zone, uint32_t hash_seed)
       : string_table_(AstRawStringCompare),
         values_(nullptr),
         strings_(nullptr),
         strings_end_(&strings_),
-        string_constants_(string_constants),
         zone_(zone),
         hash_seed_(hash_seed) {
-#define F(name) name##_ = nullptr;
+#define F(name, str) name##_string_ = NULL;
+    STRING_CONSTANTS(F)
+#undef F
+#define F(name) name##_ = NULL;
     OTHER_CONSTANTS(F)
 #undef F
-    DCHECK_EQ(hash_seed, string_constants->hash_seed());
     std::fill(smis_, smis_ + arraysize(smis_), nullptr);
     std::fill(one_character_strings_,
               one_character_strings_ + arraysize(one_character_strings_),
               nullptr);
-    InitializeStringConstants();
   }
 
   Zone* zone() const { return zone_; }
@@ -418,9 +378,15 @@ class AstValueFactory {
 
   void Internalize(Isolate* isolate);
 
-#define F(name, str)                           \
-  const AstRawString* name##_string() {        \
-    return string_constants_->name##_string(); \
+#define F(name, str)                                                    \
+  const AstRawString* name##_string() {                                 \
+    if (name##_string_ == NULL) {                                       \
+      const char* data = str;                                           \
+      name##_string_ = GetOneByteString(                                \
+          Vector<const uint8_t>(reinterpret_cast<const uint8_t*>(data), \
+                                static_cast<int>(strlen(data))));       \
+    }                                                                   \
+    return name##_string_;                                              \
   }
   STRING_CONSTANTS(F)
 #undef F
@@ -461,17 +427,6 @@ class AstValueFactory {
   AstRawString* GetString(uint32_t hash, bool is_one_byte,
                           Vector<const byte> literal_bytes);
 
-  void InitializeStringConstants() {
-#define F(name, str)                                                    \
-  AstRawString* raw_string_##name = string_constants_->name##_string(); \
-  base::HashMap::Entry* entry_##name = string_table_.LookupOrInsert(    \
-      raw_string_##name, raw_string_##name->hash());                    \
-  DCHECK(entry_##name->value == nullptr);                               \
-  entry_##name->value = reinterpret_cast<void*>(1);
-    STRING_CONSTANTS(F)
-#undef F
-  }
-
   static bool AstRawStringCompare(void* a, void* b);
 
   // All strings are copied here, one after another (no NULLs inbetween).
@@ -485,9 +440,6 @@ class AstValueFactory {
   AstString* strings_;
   AstString** strings_end_;
 
-  // Holds constant string values which are shared across the isolate.
-  AstStringConstants* string_constants_;
-
   // Caches for faster access: small numbers, one character lowercase strings
   // (for minified code).
   AstValue* smis_[kMaxCachedSmi + 1];
@@ -497,6 +449,10 @@ class AstValueFactory {
 
   uint32_t hash_seed_;
 
+#define F(name, str) const AstRawString* name##_string_;
+  STRING_CONSTANTS(F)
+#undef F
+
 #define F(name) AstValue* name##_;
   OTHER_CONSTANTS(F)
 #undef F

src/heap-symbols.h

@@ -6,7 +6,6 @@
 #define V8_HEAP_SYMBOLS_H_
 
 #define INTERNALIZED_STRING_LIST(V)                                \
-  V(anonymous_function_string, "(anonymous function)")             \
   V(anonymous_string, "anonymous")                                 \
   V(apply_string, "apply")                                         \
   V(arguments_string, "arguments")                                 \
@@ -15,8 +14,6 @@
   V(Array_string, "Array")                                         \
   V(ArrayIterator_string, "Array Iterator")                        \
   V(assign_string, "assign")                                       \
-  V(async_string, "async")                                         \
-  V(await_string, "await")                                         \
   V(array_to_string, "[object Array]")                             \
   V(boolean_to_string, "[object Boolean]")                         \
   V(date_to_string, "[object Date]")                               \
@@ -60,12 +57,7 @@
   V(did_handle_string, "didHandle")                                \
   V(display_name_string, "displayName")                            \
   V(done_string, "done")                                           \
-  V(dot_catch_string, ".catch")                                    \
-  V(dot_for_string, ".for")                                        \
-  V(dot_generator_object_string, ".generator_object")              \
-  V(dot_iterator_string, ".iterator")                              \
   V(dot_result_string, ".result")                                  \
-  V(dot_switch_tag_string, ".switch_tag")                          \
   V(dot_string, ".")                                               \
   V(exec_string, "exec")                                           \
   V(entries_string, "entries")                                     \
@@ -86,7 +78,6 @@
   V(getOwnPropertyDescriptors_string, "getOwnPropertyDescriptors") \
   V(getPrototypeOf_string, "getPrototypeOf")                       \
   V(get_string, "get")                                             \
-  V(get_space_string, "get ")                                      \
   V(global_string, "global")                                       \
   V(has_string, "has")                                             \
   V(hour_string, "hour")                                           \
@@ -109,7 +100,6 @@
   V(keys_string, "keys")                                           \
   V(lastIndex_string, "lastIndex")                                 \
   V(length_string, "length")                                       \
-  V(let_string, "let")                                             \
   V(line_string, "line")                                           \
   V(literal_string, "literal")                                     \
   V(Map_string, "Map")                                             \
@@ -120,9 +110,7 @@
   V(month_string, "month")                                         \
   V(multiline_string, "multiline")                                 \
   V(name_string, "name")                                           \
-  V(native_string, "native")                                       \
   V(nan_string, "NaN")                                             \
-  V(new_target_string, ".new.target")                              \
   V(next_string, "next")                                           \
   V(not_equal, "not-equal")                                        \
   V(null_string, "null")                                           \
@@ -147,11 +135,9 @@
   V(RegExp_string, "RegExp")                                       \
   V(reject_string, "reject")                                       \
   V(resolve_string, "resolve")                                     \
-  V(return_string, "return")                                       \
   V(script_string, "script")                                       \
   V(second_string, "second")                                       \
   V(setPrototypeOf_string, "setPrototypeOf")                       \
-  V(set_space_string, "set ")                                      \
   V(set_string, "set")                                             \
   V(Set_string, "Set")                                             \
   V(source_mapping_url_string, "source_mapping_url")               \
@@ -160,7 +146,6 @@
   V(source_url_string, "source_url")                               \
   V(stack_string, "stack")                                         \
   V(stackTraceLimit_string, "stackTraceLimit")                     \
-  V(star_default_star_string, "*default*")                         \
   V(sticky_string, "sticky")                                       \
   V(strict_compare_ic_string, "===")                               \
   V(string_string, "string")                                       \
@@ -170,7 +155,6 @@
   V(symbol_species_string, "[Symbol.species]")                     \
   V(SyntaxError_string, "SyntaxError")                             \
   V(then_string, "then")                                           \
-  V(this_function_string, ".this_function")                        \
   V(this_string, "this")                                           \
   V(throw_string, "throw")                                         \
   V(timed_out, "timed-out")                                        \
@@ -192,8 +176,6 @@
   V(undefined_string, "undefined")                                 \
   V(undefined_to_string, "[object Undefined]")                     \
   V(unicode_string, "unicode")                                     \
-  V(use_asm_string, "use asm")                                     \
-  V(use_strict_string, "use strict")                               \
   V(URIError_string, "URIError")                                   \
   V(valueOf_string, "valueOf")                                     \
   V(values_string, "values")                                       \

src/interpreter/bytecode-generator.cc

@@ -587,10 +587,13 @@ BytecodeGenerator::BytecodeGenerator(CompilationInfo* info)
       loop_depth_(0),
       home_object_symbol_(info->isolate()->factory()->home_object_symbol()),
       iterator_symbol_(info->isolate()->factory()->iterator_symbol()),
-      prototype_string_(info->isolate()->factory()->prototype_string()),
-      empty_fixed_array_(info->isolate()->factory()->empty_fixed_array()),
-      undefined_string_(
-          info->isolate()->ast_string_constants()->undefined_string()) {}
+      empty_fixed_array_(info->isolate()->factory()->empty_fixed_array()) {
+  AstValueFactory* ast_value_factory = info->parse_info()->ast_value_factory();
+  const AstRawString* prototype_string = ast_value_factory->prototype_string();
+  ast_value_factory->Internalize(info->isolate());
+  prototype_string_ = prototype_string->string();
+  undefined_string_ = ast_value_factory->undefined_string();
+}
 
 Handle<BytecodeArray> BytecodeGenerator::FinalizeBytecode(Isolate* isolate) {
   AllocateDeferredConstants(isolate);

src/isolate.cc

@@ -9,7 +9,6 @@
 #include <fstream>  // NOLINT(readability/streams)
 #include <sstream>
 
-#include "src/ast/ast-value-factory.h"
 #include "src/ast/context-slot-cache.h"
 #include "src/base/hashmap.h"
 #include "src/base/platform/platform.h"
@@ -2376,9 +2375,6 @@ void Isolate::Deinit() {
   delete interpreter_;
   interpreter_ = NULL;
 
-  delete ast_string_constants_;
-  ast_string_constants_ = nullptr;
-
   delete cpu_profiler_;
   cpu_profiler_ = NULL;
 
@@ -2702,11 +2698,6 @@ bool Isolate::Init(Deserializer* des) {
 
   time_millis_at_init_ = heap_.MonotonicallyIncreasingTimeInMs();
 
-  {
-    HandleScope scope(this);
-    ast_string_constants_ = new AstStringConstants(this, heap()->HashSeed());
-  }
-
   if (!create_heap_objects) {
     // Now that the heap is consistent, it's OK to generate the code for the
     // deopt entry table that might have been referred to by optimized code in

src/isolate.h

@@ -35,7 +35,6 @@ namespace internal {
 
 class AccessCompilerData;
 class AddressToIndexHashMap;
-class AstStringConstants;
 class BasicBlockProfiler;
 class Bootstrapper;
 class CancelableTaskManager;
@@ -1158,10 +1157,6 @@ class Isolate {
     return cancelable_task_manager_;
   }
 
-  AstStringConstants* ast_string_constants() const {
-    return ast_string_constants_;
-  }
-
   interpreter::Interpreter* interpreter() const { return interpreter_; }
 
   AccountingAllocator* allocator() { return allocator_; }
@@ -1404,8 +1399,6 @@ class Isolate {
   std::unique_ptr<CodeEventDispatcher> code_event_dispatcher_;
   FunctionEntryHook function_entry_hook_;
 
-  AstStringConstants* ast_string_constants_;
-
   interpreter::Interpreter* interpreter_;
 
   CompilerDispatcher* compiler_dispatcher_;

src/parsing/parser.cc

@@ -559,8 +559,7 @@ Parser::Parser(ParseInfo* info)
   }
   if (info->ast_value_factory() == NULL) {
     // info takes ownership of AstValueFactory.
-    info->set_ast_value_factory(new AstValueFactory(
-        zone(), info->isolate()->ast_string_constants(), info->hash_seed()));
+    info->set_ast_value_factory(new AstValueFactory(zone(), info->hash_seed()));
     info->set_ast_value_factory_owned();
     ast_value_factory_ = info->ast_value_factory();
     ast_node_factory_.set_ast_value_factory(ast_value_factory_);

test/cctest/asmjs/test-asm-typer.cc

@@ -47,8 +47,7 @@ class AsmTyperHarnessBuilder {
         handles_(),
         zone_(handles_.main_zone()),
         isolate_(CcTest::i_isolate()),
-        ast_value_factory_(zone_, isolate_->ast_string_constants(),
-                           isolate_->heap()->HashSeed()),
+        ast_value_factory_(zone_, isolate_->heap()->HashSeed()),
         factory_(isolate_->factory()),
         source_code_(
             factory_->NewStringFromUtf8(CStrVector(source)).ToHandleChecked()),

test/cctest/test-ast.cc

@@ -30,7 +30,6 @@
 #include "src/v8.h"
 
 #include "src/ast/ast.h"
-#include "src/isolate.h"
 #include "src/objects-inl.h"
 #include "src/zone/accounting-allocator.h"
 #include "test/cctest/cctest.h"
@@ -38,16 +37,12 @@
 using namespace v8::internal;
 
 TEST(List) {
-  v8::V8::Initialize();
-  Isolate* isolate = CcTest::i_isolate();
-
   List<AstNode*>* list = new List<AstNode*>(0);
   CHECK_EQ(0, list->length());
 
   v8::internal::AccountingAllocator allocator;
   Zone zone(&allocator, ZONE_NAME);
-  AstValueFactory value_factory(&zone, isolate->ast_string_constants(),
-                                isolate->heap()->HashSeed());
+  AstValueFactory value_factory(&zone, 0);
   AstNodeFactory factory(&value_factory);
   AstNode* node = factory.NewEmptyStatement(kNoSourcePosition);
   list->Add(node);

test/cctest/test-parsing.cc

@@ -175,8 +175,7 @@ TEST(ScanHTMLEndComments) {
     scanner.Initialize(stream.get());
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
-        &zone, CcTest::i_isolate()->ast_string_constants(),
-        CcTest::i_isolate()->heap()->HashSeed());
+        &zone, CcTest::i_isolate()->heap()->HashSeed());
     i::PendingCompilationErrorHandler pending_error_handler;
     i::PreParser preparser(
         &zone, &scanner, stack_limit, &ast_value_factory,
@@ -194,8 +193,7 @@ TEST(ScanHTMLEndComments) {
     scanner.Initialize(stream.get());
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
-        &zone, CcTest::i_isolate()->ast_string_constants(),
-        CcTest::i_isolate()->heap()->HashSeed());
+        &zone, CcTest::i_isolate()->heap()->HashSeed());
     i::PendingCompilationErrorHandler pending_error_handler;
     i::PreParser preparser(
         &zone, &scanner, stack_limit, &ast_value_factory,
@@ -366,8 +364,7 @@ TEST(StandAlonePreParser) {
 
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
-        &zone, CcTest::i_isolate()->ast_string_constants(),
-        CcTest::i_isolate()->heap()->HashSeed());
+        &zone, CcTest::i_isolate()->heap()->HashSeed());
     i::PendingCompilationErrorHandler pending_error_handler;
     i::PreParser preparser(
         &zone, &scanner, stack_limit, &ast_value_factory,
@@ -403,8 +400,7 @@ TEST(StandAlonePreParserNoNatives) {
     // Preparser defaults to disallowing natives syntax.
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
-        &zone, CcTest::i_isolate()->ast_string_constants(),
-        CcTest::i_isolate()->heap()->HashSeed());
+        &zone, CcTest::i_isolate()->heap()->HashSeed());
     i::PendingCompilationErrorHandler pending_error_handler;
     i::PreParser preparser(&zone, &scanner, stack_limit, &ast_value_factory,
                            &pending_error_handler,
@@ -470,9 +466,8 @@ TEST(RegressChromium62639) {
   i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
   scanner.Initialize(stream.get());
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
-  i::AstValueFactory ast_value_factory(
-      &zone, CcTest::i_isolate()->ast_string_constants(),
-      CcTest::i_isolate()->heap()->HashSeed());
+  i::AstValueFactory ast_value_factory(&zone,
+                                       CcTest::i_isolate()->heap()->HashSeed());
   i::PendingCompilationErrorHandler pending_error_handler;
   i::PreParser preparser(&zone, &scanner,
                          CcTest::i_isolate()->stack_guard()->real_climit(),
@@ -546,9 +541,8 @@ TEST(PreParseOverflow) {
   scanner.Initialize(stream.get());
 
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
-  i::AstValueFactory ast_value_factory(
-      &zone, CcTest::i_isolate()->ast_string_constants(),
-      CcTest::i_isolate()->heap()->HashSeed());
+  i::AstValueFactory ast_value_factory(&zone,
+                                       CcTest::i_isolate()->heap()->HashSeed());
   i::PendingCompilationErrorHandler pending_error_handler;
   i::PreParser preparser(&zone, &scanner, stack_limit, &ast_value_factory,
                          &pending_error_handler,
@@ -648,9 +642,8 @@ void TestScanRegExp(const char* re_source, const char* expected) {
   CHECK(scanner.ScanRegExpPattern());
   scanner.Next();  // Current token is now the regexp literal.
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
-  i::AstValueFactory ast_value_factory(
-      &zone, CcTest::i_isolate()->ast_string_constants(),
-      CcTest::i_isolate()->heap()->HashSeed());
+  i::AstValueFactory ast_value_factory(&zone,
+                                       CcTest::i_isolate()->heap()->HashSeed());
   const i::AstRawString* current_symbol =
       scanner.CurrentSymbol(&ast_value_factory);
   ast_value_factory.Internalize(CcTest::i_isolate());
@@ -1337,8 +1330,7 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
         i::ScannerStream::For(source));
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
-        &zone, CcTest::i_isolate()->ast_string_constants(),
-        CcTest::i_isolate()->heap()->HashSeed());
+        &zone, CcTest::i_isolate()->heap()->HashSeed());
     i::PreParser preparser(&zone, &scanner, stack_limit, &ast_value_factory,
                            &pending_error_handler,
                            isolate->counters()->runtime_call_stats());
@@ -3172,8 +3164,7 @@ TEST(SerializationOfMaybeAssignmentFlag) {
   i::Handle<i::Object> o = v8::Utils::OpenHandle(*v);
   i::Handle<i::JSFunction> f = i::Handle<i::JSFunction>::cast(o);
   i::Context* context = f->context();
-  i::AstValueFactory avf(&zone, isolate->ast_string_constants(),
-                         isolate->heap()->HashSeed());
+  i::AstValueFactory avf(&zone, isolate->heap()->HashSeed());
   const i::AstRawString* name = avf.GetOneByteString("result");
   avf.Internalize(isolate);
   i::Handle<i::String> str = name->string();
@@ -3222,8 +3213,7 @@ TEST(IfArgumentsArrayAccessedThenParametersMaybeAssigned) {
   i::Handle<i::Object> o = v8::Utils::OpenHandle(*v);
   i::Handle<i::JSFunction> f = i::Handle<i::JSFunction>::cast(o);
   i::Context* context = f->context();
-  i::AstValueFactory avf(&zone, isolate->ast_string_constants(),
-                         isolate->heap()->HashSeed());
+  i::AstValueFactory avf(&zone, isolate->heap()->HashSeed());
   const i::AstRawString* name_x = avf.GetOneByteString("x");
   avf.Internalize(isolate);
 
@@ -3519,8 +3509,7 @@ namespace {
 
 i::Scope* DeserializeFunctionScope(i::Isolate* isolate, i::Zone* zone,
                                    i::Handle<i::JSObject> m, const char* name) {
-  i::AstValueFactory avf(zone, isolate->ast_string_constants(),
-                         isolate->heap()->HashSeed());
+  i::AstValueFactory avf(zone, isolate->heap()->HashSeed());
   i::Handle<i::JSFunction> f = i::Handle<i::JSFunction>::cast(
       i::JSReceiver::GetProperty(isolate, m, name).ToHandleChecked());
   i::DeclarationScope* script_scope =