Revert "[objects] Remove MakeExternal case for uncached internal strings"

This reverts commit 3a6f75ac.

Reason for revert: performance regressions https://bugs.chromium.org/p/chromium/issues/detail?id=1163063

Original change's description:
> [objects] Remove MakeExternal case for uncached internal strings
>
> Concurrently accessing internal external uncached strings is not
> thread-safe. We are removing a case where we can make such a string
> through MakeExternal.
>
> Bug: v8:7790
> Change-Id: I958062c15cf40ccc330600bb572de98620866e54
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565511
> Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
> Reviewed-by: Leszek Swirski <leszeks@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#71573}

TBR=leszeks@chromium.org,solanes@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: v8:7790
Change-Id: I5dcc734869c3c921eacd89426309141127a85f47
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2633547Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72123}

src/objects/string.cc

@@ -182,9 +182,7 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   ReadOnlyRoots roots(isolate);
   if (size < ExternalString::kSizeOfAllExternalStrings) {
     if (is_internalized) {
-      // We do not support this case since accessing internal external
-      // uncached strings is not thread-safe.
-      return false;
+      new_map = roots.uncached_external_internalized_string_map();
     } else {
       new_map = roots.uncached_external_string_map();
     }
@@ -261,13 +259,9 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   Map new_map;
   ReadOnlyRoots roots(isolate);
   if (size < ExternalString::kSizeOfAllExternalStrings) {
-    if (is_internalized) {
-      // We do not support this case since accessing internal external
-      // uncached strings is not thread-safe.
-      return false;
-    } else {
-      new_map = roots.uncached_external_one_byte_string_map();
-    }
+    new_map = is_internalized
+                  ? roots.uncached_external_one_byte_internalized_string_map()
+                  : roots.uncached_external_one_byte_string_map();
   } else {
     new_map = is_internalized
                   ? roots.external_one_byte_internalized_string_map()
@@ -315,11 +309,6 @@ bool String::SupportsExternalization() {
   DCHECK_LE(ExternalString::kUncachedSize, this->Size());
 #endif
 
-  if (this->Size() < ExternalString::kSizeOfAllExternalStrings &&
-      this->IsInternalizedString()) {
-    return false;
-  }
-
   Isolate* isolate = GetIsolateFromWritableObject(*this);
   return !isolate->heap()->IsInGCPostProcessing();
 }

src/objects/string.h

@@ -361,7 +361,7 @@ class String : public TorqueGeneratedString<String, Name> {
       v8::String::ExternalStringResource* resource);
   V8_EXPORT_PRIVATE bool MakeExternal(
       v8::String::ExternalOneByteStringResource* resource);
-  V8_EXPORT_PRIVATE bool SupportsExternalization();
+  bool SupportsExternalization();
 
   // Conversion.
   // "array index": an index allowed by the ES spec for JSArrays.

test/cctest/heap/test-heap.cc

@@ -5915,7 +5915,7 @@ TEST(Regress631969) {
 
   {
     StaticOneByteResource external_string("12345678901234");
-    CHECK(s3->MakeExternal(&external_string));
+    s3->MakeExternal(&external_string);
     CcTest::CollectGarbage(OLD_SPACE);
     // This avoids the GC from trying to free stack allocated resources.
     i::Handle<i::ExternalOneByteString>::cast(s3)->SetResource(isolate,

test/cctest/test-api.cc

@@ -16877,7 +16877,7 @@ TEST(VisitExternalStrings) {
   v8::Isolate* isolate = CcTest::isolate();
   LocalContext env;
   v8::HandleScope scope(isolate);
-  const char string[] = "Some string that's long";
+  const char string[] = "Some string";
   uint16_t* two_byte_string = AsciiToTwoByteString(string);
   TestResource* resource[4];
   resource[0] = new TestResource(two_byte_string);
@@ -16957,7 +16957,7 @@ TEST(ExternalInternalizedStringCollectedAtTearDown) {
     v8::Local<v8::String> ring =
         CompileRun("ring")->ToString(env.local()).ToLocalChecked();
     CHECK(v8::Utils::OpenHandle(*ring)->IsInternalizedString());
-    CHECK(ring->MakeExternal(inscription));
+    ring->MakeExternal(inscription);
     // Ring is still alive.  Orcs are roaming freely across our lands.
     CHECK_EQ(0, destroyed);
     USE(ring);
@@ -16979,7 +16979,7 @@ TEST(ExternalInternalizedStringCollectedAtGC) {
         new TestOneByteResource(i::StrDup(s), &destroyed);
     v8::Local<v8::String> ring = CompileRun("ring").As<v8::String>();
     CHECK(v8::Utils::OpenHandle(*ring)->IsInternalizedString());
-    CHECK(ring->MakeExternal(inscription));
+    ring->MakeExternal(inscription);
     // Ring is still alive.  Orcs are roaming freely across our lands.
     CHECK_EQ(0, destroyed);
     USE(ring);
@@ -18091,7 +18091,7 @@ THREADED_TEST(TwoByteStringInOneByteCons) {
 
   TestResource resource(uc16_buffer);
 
-  CHECK(flat_string->MakeExternal(&resource));
+  flat_string->MakeExternal(&resource);
 
   CHECK(flat_string->IsTwoByteRepresentation());
 
@@ -21442,7 +21442,7 @@ class RegExpInterruptTest {
     v8::Local<v8::String> string =
         v8::Local<v8::String>::New(isolate, instance->string_handle_);
     CHECK(string->CanMakeExternal());
-    CHECK(string->MakeExternal(&one_byte_string_resource));
+    string->MakeExternal(&one_byte_string_resource);
   }
 
   static void MakeSubjectTwoByteExternal(v8::Isolate* isolate, void* data) {
@@ -21452,7 +21452,7 @@ class RegExpInterruptTest {
     v8::Local<v8::String> string =
         v8::Local<v8::String>::New(isolate, instance->string_handle_);
     CHECK(string->CanMakeExternal());
-    CHECK(string->MakeExternal(&two_byte_string_resource));
+    string->MakeExternal(&two_byte_string_resource);
   }
 
   static void ReenterIrregexp(v8::Isolate* isolate, void* data) {

test/cctest/test-concurrent-string.cc

@@ -19,10 +19,10 @@ namespace internal {
 
 namespace {
 
-#define DOUBLE_VALUE 12345.123456789
-#define STRING_VALUE "12345.123456789"
+#define DOUBLE_VALUE 28.123456789
+#define STRING_VALUE "28.123456789"
 #define ARRAY_VALUE \
-  { '1', '2', '3', '4', '5', '.', '1', '2', '3', '4', '5', '6', '7', '8', '9' }
+  { '2', '8', '.', '1', '2', '3', '4', '5', '6', '7', '8', '9' }
 
 // Adapted from cctest/test-api.cc, and
 // test/cctest/heap/test-external-string-tracker.cc.
@@ -202,8 +202,7 @@ TEST(InspectTwoByteExternalizing) {
   // TODO(solanes): Can we have only one raw string?
   const char* raw_string = STRING_VALUE;
   // TODO(solanes): Is this the best way to create a two byte string from chars?
-  const int kLength = 15;
-  DCHECK_EQ(kLength, strlen(raw_string));
+  const int kLength = 12;
   const uint16_t two_byte_array[kLength] = ARRAY_VALUE;
   Handle<String> two_bytes_string;
   {

test/cctest/test-serialize.cc

@@ -2114,28 +2114,28 @@ TEST(CodeSerializerExternalString) {
   v8::HandleScope scope(CcTest::isolate());
 
   // Obtain external internalized one-byte string.
-  SerializerOneByteResource one_byte_resource("one_byte_but_long", 17);
+  SerializerOneByteResource one_byte_resource("one_byte", 8);
   Handle<String> one_byte_string =
-      isolate->factory()->NewStringFromAsciiChecked("one_byte_but_long");
+      isolate->factory()->NewStringFromAsciiChecked("one_byte");
   one_byte_string = isolate->factory()->InternalizeString(one_byte_string);
   one_byte_string->MakeExternal(&one_byte_resource);
   CHECK(one_byte_string->IsExternalOneByteString());
   CHECK(one_byte_string->IsInternalizedString());
 
   // Obtain external internalized two-byte string.
-  SerializerTwoByteResource two_byte_resource("two_byte_but_long", 17);
+  SerializerTwoByteResource two_byte_resource("two_byte", 8);
   Handle<String> two_byte_string =
-      isolate->factory()->NewStringFromAsciiChecked("two_byte_but_long");
+      isolate->factory()->NewStringFromAsciiChecked("two_byte");
   two_byte_string = isolate->factory()->InternalizeString(two_byte_string);
   two_byte_string->MakeExternal(&two_byte_resource);
   CHECK(two_byte_string->IsExternalTwoByteString());
   CHECK(two_byte_string->IsInternalizedString());
 
   const char* source =
-      "var o = {}                                 \n"
-      "o.one_byte_but_long = 7;                   \n"
-      "o.two_byte_but_long = 8;                   \n"
-      "o.one_byte_but_long + o.two_byte_but_long; \n";
+      "var o = {}               \n"
+      "o.one_byte = 7;          \n"
+      "o.two_byte = 8;          \n"
+      "o.one_byte + o.two_byte; \n";
   Handle<String> source_string = isolate->factory()
                                      ->NewStringFromUtf8(CStrVector(source))
                                      .ToHandleChecked();

test/cctest/test-strings.cc

@@ -39,7 +39,6 @@
 #include "src/heap/heap-inl.h"
 #include "src/init/v8.h"
 #include "src/objects/objects-inl.h"
-#include "src/objects/string.h"
 #include "src/strings/unicode-decoder.h"
 #include "test/cctest/cctest.h"
 #include "test/cctest/heap/heap-utils.h"
@@ -1902,6 +1901,20 @@ TEST(StringEquals) {
   CHECK(!bar_str->StringEquals(foo_str2));
 }
 
+class OneByteStringResource : public v8::String::ExternalOneByteStringResource {
+ public:
+  // Takes ownership of |data|.
+  OneByteStringResource(char* data, size_t length)
+      : data_(data), length_(length) {}
+  ~OneByteStringResource() override { delete[] data_; }
+  const char* data() const override { return data_; }
+  size_t length() const override { return length_; }
+
+ private:
+  char* data_;
+  size_t length_;
+};
+
 TEST(Regress876759) {
   // Thin strings are used in conjunction with young gen
   if (FLAG_single_generation) return;
@@ -1936,9 +1949,9 @@ TEST(Regress876759) {
   Handle<String> grandparent =
       handle(ThinString::cast(*parent).actual(), isolate);
   CHECK_EQ(*parent, SlicedString::cast(*sliced).parent());
-  OneByteResource* resource =
-      new OneByteResource(external_one_byte_buf, kLength);
-  CHECK(grandparent->MakeExternal(resource));
+  OneByteStringResource* resource =
+      new OneByteStringResource(external_one_byte_buf, kLength);
+  grandparent->MakeExternal(resource);
   // The grandparent string becomes one-byte, but the child strings are still
   // two-byte.
   CHECK(grandparent->IsOneByteRepresentation());
@@ -1948,69 +1961,6 @@ TEST(Regress876759) {
   CHECK(String::IsOneByteRepresentationUnderneath(*sliced));
 }
 
-// Show failure when trying to create and internal, external and uncached string
-// through MakeExternal. One byte version.
-TEST(MakeExternalCreationFailureOneByte) {
-  Isolate* isolate = CcTest::i_isolate();
-  Factory* factory = isolate->factory();
-// Due to different size restrictions the string needs to be small but not too
-// small. One of these restrictions is whether pointer compression is enabled.
-#ifdef V8_COMPRESS_POINTERS
-  const char* raw_small = "small string";
-#elif V8_TARGET_ARCH_32_BIT
-  const char* raw_small = "smol";
-#else
-  const char* raw_small = "smalls";
-#endif  // V8_COMPRESS_POINTERS
-
-  HandleScope handle_scope(isolate);
-  Handle<String> one_byte_string =
-      factory->InternalizeString(factory->NewStringFromAsciiChecked(raw_small));
-  CHECK(one_byte_string->IsOneByteRepresentation());
-  CHECK(!one_byte_string->IsExternalString());
-  CHECK(!one_byte_string->SupportsExternalization());
-}
-
-// Show failure when trying to create and internal, external and uncached string
-// through MakeExternal. Two byte version.
-TEST(MakeExternalCreationFailureTwoByte) {
-  Isolate* isolate = CcTest::i_isolate();
-  Factory* factory = isolate->factory();
-  // Due to different size restrictions the string needs to be small but not too
-  // small.
-  const char* raw_small = "smalls";
-  const int kLength = 6;
-  DCHECK_EQ(kLength, strlen(raw_small));
-  const uint16_t two_byte_array[kLength] = {'s', 'm', 'a', 'l', 'l', 's'};
-
-  HandleScope handle_scope(isolate);
-  Handle<String> two_bytes_string;
-  {
-    Handle<SeqTwoByteString> raw =
-        factory->NewRawTwoByteString(kLength).ToHandleChecked();
-    DisallowGarbageCollection no_gc;
-    CopyChars(raw->GetChars(no_gc), two_byte_array, kLength);
-    two_bytes_string = raw;
-  }
-  two_bytes_string = factory->InternalizeString(two_bytes_string);
-  CHECK(two_bytes_string->IsTwoByteRepresentation());
-  CHECK(!two_bytes_string->IsExternalString());
-  if (COMPRESS_POINTERS_BOOL) {
-    CHECK(!two_bytes_string->SupportsExternalization());
-  } else {
-    // Without pointer compression, there is no string size that can cause a
-    // failure for a two byte string. It needs to be bigger than 5 chars to
-    // support externalization, but at that point is bigger than the limit and
-    // it is not uncached anymore.
-    // As a note, since pointer compression is only enabled for 64 bits, all
-    // target 32 bit archs fall in this case.
-    CHECK(two_bytes_string->MakeExternal(
-        new Resource(AsciiToTwoByteString(raw_small), strlen(raw_small))));
-    auto external = Handle<ExternalString>::cast(two_bytes_string);
-    CHECK(!external->is_uncached());
-  }
-}
-
 // Show that it is possible to internalize an external string without a copy, as
 // long as it is not uncached.
 TEST(InternalizeExternalString) {
@@ -2061,91 +2011,6 @@ TEST(InternalizeExternalStringTwoByte) {
   CHECK(string.equals(internal));
 }
 
-class UncachedExternalOneByteResource
-    : public v8::String::ExternalOneByteStringResource {
- public:
-  explicit UncachedExternalOneByteResource(const char* data)
-      : data_(data), length_(strlen(data)) {}
-
-  ~UncachedExternalOneByteResource() override { i::DeleteArray(data_); }
-
-  const char* data() const override { return data_; }
-  size_t length() const override { return length_; }
-  bool IsCacheable() const override { return false; }
-
- private:
-  const char* data_;
-  size_t length_;
-};
-
-// Show that we can internalize an external uncached string, by creating a copy.
-TEST(InternalizeExternalStringUncachedWithCopy) {
-  CcTest::InitializeVM();
-  Factory* factory = CcTest::i_isolate()->factory();
-  v8::HandleScope scope(CcTest::isolate());
-
-  // Create the string.
-  const char* raw_string = "external";
-  UncachedExternalOneByteResource* resource =
-      new UncachedExternalOneByteResource(i::StrDup(raw_string));
-  Handle<String> string =
-      factory->NewExternalStringFromOneByte(resource).ToHandleChecked();
-  CHECK(string->IsExternalString());
-
-  // Check it is uncached.
-  Handle<ExternalString> external = Handle<ExternalString>::cast(string);
-  CHECK(external->is_uncached());
-
-  // Internalize succesfully, with a copy.
-  Handle<String> internal = factory->InternalizeString(external);
-  CHECK(!external->IsInternalizedString());
-  CHECK(internal->IsInternalizedString());
-}
-
-class UncachedExternalResource : public v8::String::ExternalStringResource {
- public:
-  explicit UncachedExternalResource(const uint16_t* data)
-      : data_(data), length_(0) {
-    while (data[length_]) ++length_;
-  }
-
-  ~UncachedExternalResource() override { i::DeleteArray(data_); }
-
-  const uint16_t* data() const override { return data_; }
-  size_t length() const override { return length_; }
-  bool IsCacheable() const override { return false; }
-
- private:
-  const uint16_t* data_;
-  size_t length_;
-};
-
-// Show that we can internalize an external uncached string, by creating a copy.
-// Two byte version.
-TEST(InternalizeExternalStringUncachedWithCopyTwoByte) {
-  CcTest::InitializeVM();
-  Factory* factory = CcTest::i_isolate()->factory();
-  v8::HandleScope scope(CcTest::isolate());
-
-  // Create the string.
-  const char* raw_string = "external";
-  UncachedExternalResource* resource =
-      new UncachedExternalResource(AsciiToTwoByteString(raw_string));
-  Handle<String> string =
-      factory->NewExternalStringFromTwoByte(resource).ToHandleChecked();
-  CHECK(string->IsExternalString());
-
-  // Check it is uncached.
-  Handle<ExternalString> external = Handle<ExternalString>::cast(string);
-  CHECK(external->is_uncached());
-
-  // Internalize succesfully, with a copy.
-  CHECK(!external->IsInternalizedString());
-  Handle<String> internal = factory->InternalizeString(external);
-  CHECK(!external->IsInternalizedString());
-  CHECK(internal->IsInternalizedString());
-}
-
 }  // namespace test_strings
 }  // namespace internal
 }  // namespace v8

test/mjsunit/json2.js

@@ -180,11 +180,11 @@ Object.defineProperty(non_enum, "b", { value: 2, enumerable: false });
 non_enum.c = 3;
 TestStringify('{"a":1,"c":3}', non_enum);
 
-var str = "external_string";
+var str = "external";
 try {
   externalizeString(str, true);
 } catch (e) { }
-TestStringify("\"external_string\"", str, null, 0);
+TestStringify("\"external\"", str, null, 0);
 
 var o = {};
 o.somespecialproperty = 10;

test/mjsunit/string-external-cached.js

@@ -53,7 +53,7 @@ function test() {
     assertEquals('B', charat_str[i].charAt(3*16 + 11));
   }
 
-  charat_short = "0123456789ABC";
+  charat_short = "01234";
   try {  // String can only be externalized once
     externalizeString(charat_short, true);
   } catch (ex) { }