Track *all* external strings in the external string table.

Up till now, external strings may be tracked in the string table
(for internalized strings) or the external string table, depending
on in which order internalize and externalize happened.

The internalized string table should not have to deal with external
strings, all of which should be tracked by the external string table.

R=svenpanne@chromium.org
BUG=326489
LOG=N

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

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

diff --git a/src/api.cc b/src/api.cc
index 943dbba..290fcba 100644 (file)
--- a/src/api.cc
+++ b/src/api.cc
@@ -5733,8 +5733,8 @@ bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) {
     external = obj;
   }
 
-  ASSERT(external->IsExternalString());
-  if (result && !external->IsInternalizedString()) {
+  if (result) {
+    ASSERT(external->IsExternalString());
     isolate->heap()->external_string_table()->AddString(*external);
   }
   return result;
@@ -5791,8 +5791,8 @@ bool v8::String::MakeExternal(
     external = obj;
   }
 
-  ASSERT(external->IsExternalString());
-  if (result && !external->IsInternalizedString()) {
+  if (result) {
+    ASSERT(external->IsExternalString());
     isolate->heap()->external_string_table()->AddString(*external);
   }
   return result;

diff --git a/src/heap.cc b/src/heap.cc
index 13c874a..b9e1a2b 100644 (file)
--- a/src/heap.cc
+++ b/src/heap.cc
@@ -1938,12 +1938,7 @@ void Heap::ProcessAllocationSites(WeakObjectRetainer* retainer,
 
 void Heap::VisitExternalResources(v8::ExternalResourceVisitor* visitor) {
   DisallowHeapAllocation no_allocation;
-
-  // Both the external string table and the string table may contain
-  // external strings, but neither lists them exhaustively, nor is the
-  // intersection set empty.  Therefore we iterate over the external string
-  // table first, ignoring internalized strings, and then over the
-  // internalized string table.
+  // All external strings are listed in the external string table.
 
   class ExternalStringTableVisitorAdapter : public ObjectVisitor {
    public:
@@ -1951,13 +1946,9 @@ void Heap::VisitExternalResources(v8::ExternalResourceVisitor* visitor) {
         v8::ExternalResourceVisitor* visitor) : visitor_(visitor) {}
     virtual void VisitPointers(Object** start, Object** end) {
       for (Object** p = start; p < end; p++) {
-        // Visit non-internalized external strings,
-        // since internalized strings are listed in the string table.
-        if (!(*p)->IsInternalizedString()) {
-          ASSERT((*p)->IsExternalString());
-          visitor_->VisitExternalString(Utils::ToLocal(
-              Handle<String>(String::cast(*p))));
-        }
+        ASSERT((*p)->IsExternalString());
+        visitor_->VisitExternalString(Utils::ToLocal(
+            Handle<String>(String::cast(*p))));
       }
     }
    private:
@@ -1965,25 +1956,6 @@ void Heap::VisitExternalResources(v8::ExternalResourceVisitor* visitor) {
   } external_string_table_visitor(visitor);
 
   external_string_table_.Iterate(&external_string_table_visitor);
-
-  class StringTableVisitorAdapter : public ObjectVisitor {
-   public:
-    explicit StringTableVisitorAdapter(
-        v8::ExternalResourceVisitor* visitor) : visitor_(visitor) {}
-    virtual void VisitPointers(Object** start, Object** end) {
-      for (Object** p = start; p < end; p++) {
-        if ((*p)->IsExternalString()) {
-          ASSERT((*p)->IsInternalizedString());
-          visitor_->VisitExternalString(Utils::ToLocal(
-              Handle<String>(String::cast(*p))));
-        }
-      }
-    }
-   private:
-    v8::ExternalResourceVisitor* visitor_;
-  } string_table_visitor(visitor);
-
-  string_table()->IterateElements(&string_table_visitor);
 }
 
 

diff --git a/src/heap.h b/src/heap.h
index c8ccb7f..d2d6961 100644 (file)
--- a/src/heap.h
+++ b/src/heap.h
@@ -1416,14 +1416,6 @@ class Heap {
   // Print short heap statistics.
   void PrintShortHeapStatistics();
 
-  // Makes a new internalized string object
-  // Returns Failure::RetryAfterGC(requested_bytes, space) if the allocation
-  // failed.
-  // Please note this function does not perform a garbage collection.
-  MUST_USE_RESULT MaybeObject* CreateInternalizedString(
-      const char* str, int length, int hash);
-  MUST_USE_RESULT MaybeObject* CreateInternalizedString(String* str);
-
   // Write barrier support for address[offset] = o.
   INLINE(void RecordWrite(Address address, int offset));
 

diff --git a/src/mark-compact.cc b/src/mark-compact.cc
index 7da765d..c0e1039 100644 (file)
--- a/src/mark-compact.cc
+++ b/src/mark-compact.cc
@@ -1839,6 +1839,7 @@ class RootMarkingVisitor : public ObjectVisitor {
 
 
 // Helper class for pruning the string table.
+template<bool finalize_external_strings>
 class StringTableCleaner : public ObjectVisitor {
  public:
   explicit StringTableCleaner(Heap* heap)
@@ -1850,22 +1851,20 @@ class StringTableCleaner : public ObjectVisitor {
       Object* o = *p;
       if (o->IsHeapObject() &&
           !Marking::MarkBitFrom(HeapObject::cast(o)).Get()) {
-        // Check if the internalized string being pruned is external. We need to
-        // delete the associated external data as this string is going away.
-
-        // Since no objects have yet been moved we can safely access the map of
-        // the object.
-        if (o->IsExternalString()) {
+        if (finalize_external_strings) {
+          ASSERT(o->IsExternalString());
           heap_->FinalizeExternalString(String::cast(*p));
+        } else {
+          pointers_removed_++;
         }
         // Set the entry to the_hole_value (as deleted).
         *p = heap_->the_hole_value();
-        pointers_removed_++;
       }
     }
   }
 
   int PointersRemoved() {
+    ASSERT(!finalize_external_strings);
     return pointers_removed_;
   }
 
@@ -1875,6 +1874,10 @@ class StringTableCleaner : public ObjectVisitor {
 };
 
 
+typedef StringTableCleaner<false> InternalizedStringTableCleaner;
+typedef StringTableCleaner<true> ExternalStringTableCleaner;
+
+
 // Implementation of WeakObjectRetainer for mark compact GCs. All marked objects
 // are retained.
 class MarkCompactWeakObjectRetainer : public WeakObjectRetainer {
@@ -2398,10 +2401,12 @@ void MarkCompactCollector::AfterMarking() {
   // string table.  Cannot use string_table() here because the string
   // table is marked.
   StringTable* string_table = heap()->string_table();
-  StringTableCleaner v(heap());
-  string_table->IterateElements(&v);
-  string_table->ElementsRemoved(v.PointersRemoved());
-  heap()->external_string_table_.Iterate(&v);
+  InternalizedStringTableCleaner internalized_visitor(heap());
+  string_table->IterateElements(&internalized_visitor);
+  string_table->ElementsRemoved(internalized_visitor.PointersRemoved());
+
+  ExternalStringTableCleaner external_visitor(heap());
+  heap()->external_string_table_.Iterate(&external_visitor);
   heap()->external_string_table_.CleanUp();
 
   // Process the weak references.

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 6cdc500..fdd79f1 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -17342,6 +17342,55 @@ TEST(ExternalStringCollectedAtTearDown) {
 }
 
 
+TEST(ExternalInternalizedStringCollectedAtTearDown) {
+  int destroyed = 0;
+  v8::Isolate* isolate = v8::Isolate::New();
+  { v8::Isolate::Scope isolate_scope(isolate);
+    LocalContext env(isolate);
+    v8::HandleScope handle_scope(isolate);
+    CompileRun("var ring = 'One string to test them all';");
+    const char* s = "One string to test them all";
+    TestAsciiResource* inscription =
+        new TestAsciiResource(i::StrDup(s), &destroyed);
+    v8::Local<v8::String> ring = CompileRun("ring")->ToString();
+    CHECK(v8::Utils::OpenHandle(*ring)->IsInternalizedString());
+    ring->MakeExternal(inscription);
+    // Ring is still alive.  Orcs are roaming freely across our lands.
+    CHECK_EQ(0, destroyed);
+    USE(ring);
+  }
+
+  isolate->Dispose();
+  // Ring has been destroyed.  Free Peoples of Middle-earth Rejoice.
+  CHECK_EQ(1, destroyed);
+}
+
+
+TEST(ExternalInternalizedStringCollectedAtGC) {
+  int destroyed = 0;
+  { LocalContext env;
+    v8::HandleScope handle_scope(env->GetIsolate());
+    CompileRun("var ring = 'One string to test them all';");
+    const char* s = "One string to test them all";
+    TestAsciiResource* inscription =
+        new TestAsciiResource(i::StrDup(s), &destroyed);
+    v8::Local<v8::String> ring = CompileRun("ring")->ToString();
+    CHECK(v8::Utils::OpenHandle(*ring)->IsInternalizedString());
+    ring->MakeExternal(inscription);
+    // Ring is still alive.  Orcs are roaming freely across our lands.
+    CHECK_EQ(0, destroyed);
+    USE(ring);
+  }
+
+  // Garbage collector deals swift blows to evil.
+  CcTest::i_isolate()->compilation_cache()->Clear();
+  CcTest::heap()->CollectAllAvailableGarbage();
+
+  // Ring has been destroyed.  Free Peoples of Middle-earth Rejoice.
+  CHECK_EQ(1, destroyed);
+}
+
+
 static double DoubleFromBits(uint64_t value) {
   double target;
   i::OS::MemCopy(&target, &value, sizeof(target));