[ast] Allocate cons strings on-demand

Remove AstConsString "internalization", and instead make the conversion
to heap String be on-demand with an Allocate method. We never actually
need the heapified cons string more than once, so there's no need to do
the internalization walk or do the next/string union dance in the
AstConsString class.

This also allows us to specify how we want to allocate the String at the
call site. In particular, it allows us to allocate a flat SeqString rather
rather than a ConsString. This allows us to avoid allocating ConsStrings
which will just be passed to a flatten call, and especially avoid
allocating dead ConsStrings in the off-thread old space.

Bug: chromium:1011762
Bug: chromium:1043168
Change-Id: Id851f2f7529d92ad7e5388eb22823fd6d1959cd0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2020953Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66042}

src/ast/ast-value-factory.cc

@@ -160,36 +160,87 @@ bool AstRawString::Compare(void* a, void* b) {
 }
 
 template <typename Factory>
-void AstConsString::Internalize(Factory* factory) {
+FactoryHandle<Factory, String> AstConsString::Allocate(Factory* factory) const {
   if (IsEmpty()) {
-    set_string(factory->empty_string());
-    return;
+    return factory->empty_string();
   }
-  // AstRawStrings are internalized before AstConsStrings, so
+  // AstRawStrings are internalized before AstConsStrings are allocated, so
   // AstRawString::string() will just work.
   FactoryHandle<Factory, String> tmp(segment_.string->string().get<Factory>());
   for (AstConsString::Segment* current = segment_.next; current != nullptr;
        current = current->next) {
-    tmp =
-        factory
-            ->NewConsString(
-                current->string->string().get<Factory>(), tmp,
-                // TODO(leszeks): This is to avoid memory regressions while this
-                // path is under development -- the off-thread factory doesn't
-                // support young allocations. Figure out a way to avoid memory
-                // regressions related to ConsStrings in the off-thread path.
-                std::is_same<Factory, OffThreadFactory>::value
-                    ? AllocationType::kOld
-                    : AllocationType::kYoung)
+    tmp = factory
+              ->NewConsString(current->string->string().get<Factory>(), tmp,
+                              AllocationType::kOld)
+              .ToHandleChecked();
+  }
+  return tmp;
+}
+template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE)
+    Handle<String> AstConsString::Allocate<Factory>(Factory* factory) const;
+template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE)
+    OffThreadHandle<String> AstConsString::Allocate<OffThreadFactory>(
+        OffThreadFactory* factory) const;
+
+template <typename Factory>
+FactoryHandle<Factory, String> AstConsString::AllocateFlat(
+    Factory* factory) const {
+  if (IsEmpty()) {
+    return factory->empty_string();
+  }
+  if (!segment_.next) {
+    return segment_.string->string().get<Factory>();
+  }
+
+  int result_length = 0;
+  bool is_one_byte = true;
+  for (const AstConsString::Segment* current = &segment_; current != nullptr;
+       current = current->next) {
+    result_length += current->string->length();
+    is_one_byte = is_one_byte && current->string->is_one_byte();
+  }
+
+  if (is_one_byte) {
+    FactoryHandle<Factory, SeqOneByteString> result =
+        factory->NewRawOneByteString(result_length, AllocationType::kOld)
             .ToHandleChecked();
+    DisallowHeapAllocation no_gc;
+    uint8_t* dest = result->GetChars(no_gc) + result_length;
+    for (const AstConsString::Segment* current = &segment_; current != nullptr;
+         current = current->next) {
+      int length = current->string->length();
+      dest -= length;
+      CopyChars(dest, current->string->raw_data(), length);
+    }
+    DCHECK_EQ(dest, result->GetChars(no_gc));
+    return result;
+  }
+
+  FactoryHandle<Factory, SeqTwoByteString> result =
+      factory->NewRawTwoByteString(result_length, AllocationType::kOld)
+          .ToHandleChecked();
+  DisallowHeapAllocation no_gc;
+  uint16_t* dest = result->GetChars(no_gc) + result_length;
+  for (const AstConsString::Segment* current = &segment_; current != nullptr;
+       current = current->next) {
+    int length = current->string->length();
+    dest -= length;
+    if (current->string->is_one_byte()) {
+      CopyChars(dest, current->string->raw_data(), length);
+    } else {
+      CopyChars(dest,
+                reinterpret_cast<const uint16_t*>(current->string->raw_data()),
+                length);
+    }
   }
-  set_string(tmp);
+  DCHECK_EQ(dest, result->GetChars(no_gc));
+  return result;
 }
-template EXPORT_TEMPLATE_DEFINE(
-    V8_EXPORT_PRIVATE) void AstConsString::Internalize<Factory>(Factory*
-                                                                    factory);
-template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE) void AstConsString::
-    Internalize<OffThreadFactory>(OffThreadFactory* factory);
+template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE)
+    Handle<String> AstConsString::AllocateFlat<Factory>(Factory* factory) const;
+template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE)
+    OffThreadHandle<String> AstConsString::AllocateFlat<OffThreadFactory>(
+        OffThreadFactory* factory) const;
 
 std::forward_list<const AstRawString*> AstConsString::ToRawStrings() const {
   std::forward_list<const AstRawString*> result;
@@ -275,10 +326,7 @@ const AstRawString* AstValueFactory::CloneFromOtherFactory(
 }
 
 AstConsString* AstValueFactory::NewConsString() {
-  AstConsString* new_string = new (zone_) AstConsString;
-  DCHECK_NOT_NULL(new_string);
-  AddConsString(new_string);
-  return new_string;
+  return new (zone_) AstConsString;
 }
 
 AstConsString* AstValueFactory::NewConsString(const AstRawString* str) {
@@ -300,13 +348,6 @@ void AstValueFactory::Internalize(Factory* factory) {
     current = next;
   }
 
-  // AstConsStrings refer to AstRawStrings.
-  for (AstConsString* current = cons_strings_; current != nullptr;) {
-    AstConsString* next = current->next();
-    current->Internalize(factory);
-    current = next;
-  }
-
   ResetStrings();
 }
 template EXPORT_TEMPLATE_DEFINE(

src/ast/ast-value-factory.h

@@ -144,13 +144,12 @@ class AstConsString final : public ZoneObject {
   }
 
   template <typename Factory>
-  void Internalize(Factory* factory);
+  EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
+  FactoryHandle<Factory, String> Allocate(Factory* factory) const;
 
-  // This function can be called after internalizing.
-  V8_INLINE HandleOrOffThreadHandle<String> string() const {
-    DCHECK(has_string_);
-    return string_;
-  }
+  template <typename Factory>
+  EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
+  FactoryHandle<Factory, String> AllocateFlat(Factory* factory) const;
 
   std::forward_list<const AstRawString*> ToRawStrings() const;
 
@@ -162,34 +161,19 @@ class AstConsString final : public ZoneObject {
   AstConsString* next() const { return next_; }
   AstConsString** next_location() { return &next_; }
 
-  void set_string(HandleOrOffThreadHandle<String> string) {
-    DCHECK(!string.is_null());
-    DCHECK(!has_string_);
-    string_ = string;
-#ifdef DEBUG
-    has_string_ = true;
-#endif
-  }
-
-  // {string_} is stored as Address* instead of a Handle<String> so it can be
-  // stored in a union with {next_}.
-  union {
-    AstConsString* next_;
-    HandleOrOffThreadHandle<String> string_;
-  };
+  AstConsString* next_;
 
+  // A linked list of AstRawStrings of the contents of this AstConsString.
+  // This list has several properties:
+  //
+  //   * For empty strings the string pointer is null,
+  //   * Appended raw strings are added to the head of the list, so they are in
+  //     reverse order
   struct Segment {
     const AstRawString* string;
     AstConsString::Segment* next;
   };
   Segment segment_;
-
-#ifdef DEBUG
-  // (Debug-only:) Verify the object life-cylce: Some functions may only be
-  // called after internalization (that is, after a v8::internal::String has
-  // been set); some only before.
-  bool has_string_ = false;
-#endif
 };
 
 enum class AstSymbol : uint8_t { kHomeObjectSymbol };
@@ -294,8 +278,6 @@ class AstValueFactory {
       : string_table_(string_constants->string_table()),
         strings_(nullptr),
         strings_end_(&strings_),
-        cons_strings_(nullptr),
-        cons_strings_end_(&cons_strings_),
         string_constants_(string_constants),
         empty_cons_string_(nullptr),
         zone_(zone),
@@ -346,16 +328,9 @@ class AstValueFactory {
     strings_end_ = string->next_location();
     return string;
   }
-  AstConsString* AddConsString(AstConsString* string) {
-    *cons_strings_end_ = string;
-    cons_strings_end_ = string->next_location();
-    return string;
-  }
   void ResetStrings() {
     strings_ = nullptr;
     strings_end_ = &strings_;
-    cons_strings_ = nullptr;
-    cons_strings_end_ = &cons_strings_;
   }
   V8_EXPORT_PRIVATE AstRawString* GetOneByteStringInternal(
       Vector<const uint8_t> literal);
@@ -366,12 +341,8 @@ class AstValueFactory {
   // All strings are copied here, one after another (no zeroes inbetween).
   base::CustomMatcherHashMap string_table_;
 
-  // We need to keep track of strings_ in order since cons strings require their
-  // members to be internalized first.
   AstRawString* strings_;
   AstRawString** strings_end_;
-  AstConsString* cons_strings_;
-  AstConsString** cons_strings_end_;
 
   // Holds constant string values which are shared across the isolate.
   const AstStringConstants* string_constants_;

src/ast/ast.h

@@ -2218,9 +2218,8 @@ class FunctionLiteral final : public Expression {
 
   // Empty handle means that the function does not have a shared name (i.e.
   // the name will be set dynamically after creation of the function closure).
-  MaybeHandle<String> name() const {
-    return raw_name_ ? raw_name_->string().get<Factory>()
-                     : MaybeHandle<String>();
+  MaybeHandle<String> GetName(Factory* factory) const {
+    return raw_name_ ? raw_name_->AllocateFlat(factory) : MaybeHandle<String>();
   }
   bool has_shared_name() const { return raw_name_ != nullptr; }
   const AstConsString* raw_name() const { return raw_name_; }
@@ -2278,13 +2277,13 @@ class FunctionLiteral final : public Expression {
   // Returns either name or inferred name as a cstring.
   std::unique_ptr<char[]> GetDebugName() const;
 
-  Handle<String> inferred_name() const {
+  Handle<String> GetInferredName(Factory* factory) const {
     if (!inferred_name_.is_null()) {
       DCHECK_NULL(raw_inferred_name_);
       return inferred_name_;
     }
     if (raw_inferred_name_ != nullptr) {
-      return raw_inferred_name_->string().get<Factory>();
+      return raw_inferred_name_->Allocate(factory);
     }
     UNREACHABLE();
   }

src/heap/factory.cc

@@ -3214,7 +3214,7 @@ Handle<SharedFunctionInfo> Factory::NewSharedFunctionInfoForLiteral(
     FunctionLiteral* literal, Handle<Script> script, bool is_toplevel) {
   FunctionKind kind = literal->kind();
   Handle<SharedFunctionInfo> shared = NewSharedFunctionInfoForBuiltin(
-      literal->name(), Builtins::kCompileLazy, kind);
+      literal->GetName(this), Builtins::kCompileLazy, kind);
   SharedFunctionInfo::InitFromFunctionLiteral(isolate(), shared, literal,
                                               is_toplevel);
   shared->SetScript(ReadOnlyRoots(isolate()), *script,
@@ -3291,7 +3291,7 @@ Handle<SharedFunctionInfo> Factory::NewSharedFunctionInfo(
   Handle<String> shared_name;
   bool has_shared_name = maybe_name.ToHandle(&shared_name);
   if (has_shared_name) {
-    shared_name = String::Flatten(isolate(), shared_name, AllocationType::kOld);
+    DCHECK(shared_name->IsFlat());
     shared->set_name_or_scope_info(*shared_name);
   } else {
     DCHECK_EQ(shared->name_or_scope_info(),
@@ -4061,6 +4061,8 @@ bool Factory::EmptyStringRootIsInitialized() {
 NewFunctionArgs NewFunctionArgs::ForWasm(
     Handle<String> name,
     Handle<WasmExportedFunctionData> exported_function_data, Handle<Map> map) {
+  DCHECK(name->IsFlat());
+
   NewFunctionArgs args;
   args.name_ = name;
   args.maybe_map_ = map;
@@ -4075,6 +4077,8 @@ NewFunctionArgs NewFunctionArgs::ForWasm(
 NewFunctionArgs NewFunctionArgs::ForWasm(
     Handle<String> name, Handle<WasmJSFunctionData> js_function_data,
     Handle<Map> map) {
+  DCHECK(name->IsFlat());
+
   NewFunctionArgs args;
   args.name_ = name;
   args.maybe_map_ = map;
@@ -4089,6 +4093,7 @@ NewFunctionArgs NewFunctionArgs::ForWasm(
 NewFunctionArgs NewFunctionArgs::ForBuiltin(Handle<String> name,
                                             Handle<Map> map, int builtin_id) {
   DCHECK(Builtins::IsBuiltinId(builtin_id));
+  DCHECK(name->IsFlat());
 
   NewFunctionArgs args;
   args.name_ = name;
@@ -4105,6 +4110,8 @@ NewFunctionArgs NewFunctionArgs::ForBuiltin(Handle<String> name,
 // static
 NewFunctionArgs NewFunctionArgs::ForFunctionWithoutCode(
     Handle<String> name, Handle<Map> map, LanguageMode language_mode) {
+  DCHECK(name->IsFlat());
+
   NewFunctionArgs args;
   args.name_ = name;
   args.maybe_map_ = map;
@@ -4123,6 +4130,7 @@ NewFunctionArgs NewFunctionArgs::ForBuiltinWithPrototype(
     int instance_size, int inobject_properties, int builtin_id,
     MutableMode prototype_mutability) {
   DCHECK(Builtins::IsBuiltinId(builtin_id));
+  DCHECK(name->IsFlat());
 
   NewFunctionArgs args;
   args.name_ = name;
@@ -4145,6 +4153,7 @@ NewFunctionArgs NewFunctionArgs::ForBuiltinWithPrototype(
 NewFunctionArgs NewFunctionArgs::ForBuiltinWithoutPrototype(
     Handle<String> name, int builtin_id, LanguageMode language_mode) {
   DCHECK(Builtins::IsBuiltinId(builtin_id));
+  DCHECK(name->IsFlat());
 
   NewFunctionArgs args;
   args.name_ = name;

src/init/bootstrapper.cc

@@ -9,6 +9,7 @@
 #include "src/base/ieee754.h"
 #include "src/builtins/accessors.h"
 #include "src/codegen/compiler.h"
+#include "src/common/globals.h"
 #include "src/debug/debug.h"
 #include "src/execution/isolate-inl.h"
 #include "src/execution/microtask-queue.h"
@@ -443,6 +444,7 @@ V8_NOINLINE Handle<JSFunction> SimpleCreateFunction(Isolate* isolate,
                                                     Builtins::Name call,
                                                     int len, bool adapt) {
   DCHECK(Builtins::HasJSLinkage(call));
+  name = String::Flatten(isolate, name, AllocationType::kOld);
   NewFunctionArgs args = NewFunctionArgs::ForBuiltinWithoutPrototype(
       name, call, LanguageMode::kStrict);
   Handle<JSFunction> fun = isolate->factory()->NewFunction(args);

src/objects/objects.cc

@@ -5385,11 +5385,12 @@ void SharedFunctionInfo::InitFromFunctionLiteral(
         scope_data->Serialize(shared_info->GetIsolate());
 
     data = isolate->factory()->NewUncompiledDataWithPreparseData(
-        lit->inferred_name(), lit->start_position(), lit->end_position(),
-        preparse_data);
+        lit->GetInferredName(isolate->factory()), lit->start_position(),
+        lit->end_position(), preparse_data);
   } else {
     data = isolate->factory()->NewUncompiledDataWithoutPreparseData(
-        lit->inferred_name(), lit->start_position(), lit->end_position());
+        lit->GetInferredName(isolate->factory()), lit->start_position(),
+        lit->end_position());
   }
 
   shared_info->set_uncompiled_data(*data);

test/unittests/heap/off-thread-factory-unittest.cc

@@ -153,7 +153,7 @@ TEST_F(OffThreadFactoryTest, AstConsString_CreatesConsString) {
   ast_value_factory.Internalize(off_thread_factory());
 
   OffThreadHandle<FixedArray> off_thread_wrapper =
-      WrapString(foobar_string->string().get<OffThreadFactory>());
+      WrapString(foobar_string->Allocate(off_thread_factory()));
 
   off_thread_factory()->FinishOffThread();