Allow externalizing strings in old pointer space.

This is what I think is a better solution to the "external strings in
old pointer space" problem. Basically, it is an issue because GC scans
all fields of objects in old pointer space and if the cached address
of the backing store is unaligned, it looks like a heap object, boom.

The solution here is to use short external strings when we externalize
a string in old pointer space, and when the address is unaligned.
Short external strings don't cache the address, so GC has no issues.

BUG=268686
LOG=Y
R=dcarney@chromium.org

Review URL: https://codereview.chromium.org/146183006

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19093 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/api.cc b/src/api.cc
index 064a4b0..7a412df 100644 (file)
--- a/src/api.cc
+++ b/src/api.cc
@@ -5426,23 +5426,6 @@ static i::Handle<i::String> NewExternalAsciiStringHandle(
 }
 
 
-static bool RedirectToExternalString(i::Isolate* isolate,
-                                     i::Handle<i::String> parent,
-                                     i::Handle<i::String> external) {
-  if (parent->IsConsString()) {
-    i::Handle<i::ConsString> cons = i::Handle<i::ConsString>::cast(parent);
-    cons->set_first(*external);
-    cons->set_second(isolate->heap()->empty_string());
-  } else {
-    ASSERT(parent->IsSlicedString());
-    i::Handle<i::SlicedString> slice = i::Handle<i::SlicedString>::cast(parent);
-    slice->set_parent(*external);
-    slice->set_offset(0);
-  }
-  return true;
-}
-
-
 Local<String> v8::String::NewExternal(
     Isolate* isolate,
     v8::String::ExternalStringResource* resource) {
@@ -5472,22 +5455,10 @@ bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
   }
   CHECK(resource && resource->data());
 
-  bool result;
-  i::Handle<i::String> external;
-  if (isolate->heap()->old_pointer_space()->Contains(*obj)) {
-    // We do not allow external strings in the old pointer space.  Instead of
-    // converting the string in-place, we keep the cons/sliced string and
-    // point it to a newly-allocated external string.
-    external = NewExternalStringHandle(isolate, resource);
-    result = RedirectToExternalString(isolate, obj, external);
-  } else {
-    result = obj->MakeExternal(resource);
-    external = obj;
-  }
-
+  bool result = obj->MakeExternal(resource);
   if (result) {
-    ASSERT(external->IsExternalString());
-    isolate->heap()->external_string_table()->AddString(*external);
+    ASSERT(obj->IsExternalString());
+    isolate->heap()->external_string_table()->AddString(*obj);
   }
   return result;
 }
@@ -5524,22 +5495,10 @@ bool v8::String::MakeExternal(
   }
   CHECK(resource && resource->data());
 
-  bool result;
-  i::Handle<i::String> external;
-  if (isolate->heap()->old_pointer_space()->Contains(*obj)) {
-    // We do not allow external strings in the old pointer space.  Instead of
-    // converting the string in-place, we keep the cons/sliced string and
-    // point it to a newly-allocated external string.
-    external = NewExternalAsciiStringHandle(isolate, resource);
-    result = RedirectToExternalString(isolate, obj, external);
-  } else {
-    result = obj->MakeExternal(resource);
-    external = obj;
-  }
-
+  bool result = obj->MakeExternal(resource);
   if (result) {
-    ASSERT(external->IsExternalString());
-    isolate->heap()->external_string_table()->AddString(*external);
+    ASSERT(obj->IsExternalString());
+    isolate->heap()->external_string_table()->AddString(*obj);
   }
   return result;
 }

diff --git a/src/heap-inl.h b/src/heap-inl.h
index d42e1cf..09d754f 100644 (file)
--- a/src/heap-inl.h
+++ b/src/heap-inl.h
@@ -418,7 +418,7 @@ AllocationSpace Heap::TargetSpaceId(InstanceType type) {
 }
 
 
-bool Heap::AllowedToBeMigrated(HeapObject* object, AllocationSpace dst) {
+bool Heap::AllowedToBeMigrated(HeapObject* obj, AllocationSpace dst) {
   // Object migration is governed by the following rules:
   //
   // 1) Objects in new-space can be migrated to one of the old spaces
@@ -428,18 +428,22 @@ bool Heap::AllowedToBeMigrated(HeapObject* object, AllocationSpace dst) {
   //    fixed arrays in new-space, old-data-space and old-pointer-space.
   // 4) Fillers (one word) can never migrate, they are skipped by
   //    incremental marking explicitly to prevent invalid pattern.
+  // 5) Short external strings can end up in old pointer space when a cons
+  //    string in old pointer space is made external (String::MakeExternal).
   //
   // Since this function is used for debugging only, we do not place
   // asserts here, but check everything explicitly.
-  if (object->map() == one_pointer_filler_map()) return false;
-  InstanceType type = object->map()->instance_type();
-  MemoryChunk* chunk = MemoryChunk::FromAddress(object->address());
+  if (obj->map() == one_pointer_filler_map()) return false;
+  InstanceType type = obj->map()->instance_type();
+  MemoryChunk* chunk = MemoryChunk::FromAddress(obj->address());
   AllocationSpace src = chunk->owner()->identity();
   switch (src) {
     case NEW_SPACE:
       return dst == src || dst == TargetSpaceId(type);
     case OLD_POINTER_SPACE:
-      return dst == src && (dst == TargetSpaceId(type) || object->IsFiller());
+      return dst == src &&
+          (dst == TargetSpaceId(type) || obj->IsFiller() ||
+          (obj->IsExternalString() && ExternalString::cast(obj)->is_short()));
     case OLD_DATA_SPACE:
       return dst == src && dst == TargetSpaceId(type);
     case CODE_SPACE:

diff --git a/src/objects-inl.h b/src/objects-inl.h
index dd3c0b1..18926bf 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -3193,6 +3193,7 @@ void ExternalAsciiString::update_data_cache() {
 
 void ExternalAsciiString::set_resource(
     const ExternalAsciiString::Resource* resource) {
+  ASSERT(IsAligned(reinterpret_cast<intptr_t>(resource), kPointerSize));
   *reinterpret_cast<const Resource**>(
       FIELD_ADDR(this, kResourceOffset)) = resource;
   if (resource != NULL) update_data_cache();

diff --git a/src/objects.cc b/src/objects.cc
index 41703b9..48d77db 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -1273,27 +1273,37 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   bool is_ascii = this->IsOneByteRepresentation();
   bool is_internalized = this->IsInternalizedString();
 
-  // Morph the object to an external string by adjusting the map and
-  // reinitializing the fields.
-  if (size >= ExternalString::kSize) {
+  // Morph the string to an external string by replacing the map and
+  // reinitializing the fields.  This won't work if
+  // - the space the existing string occupies is too small for a regular
+  //   external string.
+  // - the existing string is in old pointer space and the backing store of
+  //   the external string is not aligned.  The GC cannot deal with fields
+  //   containing an unaligned address that points to outside of V8's heap.
+  // In either case we resort to a short external string instead, omitting
+  // the field caching the address of the backing store.  When we encounter
+  // short external strings in generated code, we need to bailout to runtime.
+  if (size < ExternalString::kSize ||
+      (!IsAligned(reinterpret_cast<intptr_t>(resource->data()), kPointerSize) &&
+       heap->old_pointer_space()->Contains(this))) {
     this->set_map_no_write_barrier(
         is_internalized
             ? (is_ascii
-                   ? heap->external_internalized_string_with_one_byte_data_map()
-                   : heap->external_internalized_string_map())
+                ? heap->
+                    short_external_internalized_string_with_one_byte_data_map()
+                : heap->short_external_internalized_string_map())
             : (is_ascii
-                   ? heap->external_string_with_one_byte_data_map()
-                   : heap->external_string_map()));
+                ? heap->short_external_string_with_one_byte_data_map()
+                : heap->short_external_string_map()));
   } else {
     this->set_map_no_write_barrier(
         is_internalized
-          ? (is_ascii
-               ? heap->
-                   short_external_internalized_string_with_one_byte_data_map()
-               : heap->short_external_internalized_string_map())
-          : (is_ascii
-                 ? heap->short_external_string_with_one_byte_data_map()
-                 : heap->short_external_string_map()));
+            ? (is_ascii
+                ? heap->external_internalized_string_with_one_byte_data_map()
+                : heap->external_internalized_string_map())
+            : (is_ascii
+                ? heap->external_string_with_one_byte_data_map()
+                : heap->external_string_map()));
   }
   ExternalTwoByteString* self = ExternalTwoByteString::cast(this);
   self->set_resource(resource);
@@ -1334,16 +1344,26 @@ bool String::MakeExternal(v8::String::ExternalAsciiStringResource* resource) {
   }
   bool is_internalized = this->IsInternalizedString();
 
-  // Morph the object to an external string by adjusting the map and
-  // reinitializing the fields.  Use short version if space is limited.
-  if (size >= ExternalString::kSize) {
-    this->set_map_no_write_barrier(
-        is_internalized ? heap->external_ascii_internalized_string_map()
-                        : heap->external_ascii_string_map());
-  } else {
+  // Morph the string to an external string by replacing the map and
+  // reinitializing the fields.  This won't work if
+  // - the space the existing string occupies is too small for a regular
+  //   external string.
+  // - the existing string is in old pointer space and the backing store of
+  //   the external string is not aligned.  The GC cannot deal with fields
+  //   containing an unaligned address that points to outside of V8's heap.
+  // In either case we resort to a short external string instead, omitting
+  // the field caching the address of the backing store.  When we encounter
+  // short external strings in generated code, we need to bailout to runtime.
+  if (size < ExternalString::kSize ||
+      (!IsAligned(reinterpret_cast<intptr_t>(resource->data()), kPointerSize) &&
+       heap->old_pointer_space()->Contains(this))) {
     this->set_map_no_write_barrier(
         is_internalized ? heap->short_external_ascii_internalized_string_map()
                         : heap->short_external_ascii_string_map());
+  } else {
+    this->set_map_no_write_barrier(
+        is_internalized ? heap->external_ascii_internalized_string_map()
+                        : heap->external_ascii_string_map());
   }
   ExternalAsciiString* self = ExternalAsciiString::cast(this);
   self->set_resource(resource);

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index f473f5c..c49aea7 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -17862,6 +17862,50 @@ class VisitorImpl : public v8::ExternalResourceVisitor {
 };
 
 
+TEST(ExternalizeOldSpaceTwoByteCons) {
+  LocalContext env;
+  v8::HandleScope scope(env->GetIsolate());
+  v8::Local<v8::String> cons =
+      CompileRun("'Romeo Montague ' + 'Juliet Capulet'")->ToString();
+  CHECK(v8::Utils::OpenHandle(*cons)->IsConsString());
+  CcTest::heap()->CollectAllAvailableGarbage();
+  CHECK(CcTest::heap()->old_pointer_space()->Contains(
+            *v8::Utils::OpenHandle(*cons)));
+
+  TestResource* resource = new TestResource(
+      AsciiToTwoByteString("Romeo Montague Juliet Capulet"));
+  cons->MakeExternal(resource);
+
+  CHECK(cons->IsExternal());
+  CHECK_EQ(resource, cons->GetExternalStringResource());
+  String::Encoding encoding;
+  CHECK_EQ(resource, cons->GetExternalStringResourceBase(&encoding));
+  CHECK_EQ(String::TWO_BYTE_ENCODING, encoding);
+}
+
+
+TEST(ExternalizeOldSpaceOneByteCons) {
+  LocalContext env;
+  v8::HandleScope scope(env->GetIsolate());
+  v8::Local<v8::String> cons =
+      CompileRun("'Romeo Montague ' + 'Juliet Capulet'")->ToString();
+  CHECK(v8::Utils::OpenHandle(*cons)->IsConsString());
+  CcTest::heap()->CollectAllAvailableGarbage();
+  CHECK(CcTest::heap()->old_pointer_space()->Contains(
+            *v8::Utils::OpenHandle(*cons)));
+
+  TestAsciiResource* resource =
+      new TestAsciiResource(i::StrDup("Romeo Montague Juliet Capulet"));
+  cons->MakeExternal(resource);
+
+  CHECK(cons->IsExternalAscii());
+  CHECK_EQ(resource, cons->GetExternalAsciiStringResource());
+  String::Encoding encoding;
+  CHECK_EQ(resource, cons->GetExternalStringResourceBase(&encoding));
+  CHECK_EQ(String::ONE_BYTE_ENCODING, encoding);
+}
+
+
 TEST(VisitExternalStrings) {
   LocalContext env;
   v8::HandleScope scope(env->GetIsolate());