[v8-dev] Do not externalize single-character strings. (issue 462643002 by [email protected])

yangguo Mon, 11 Aug 2014 05:23:32 -0700

Reviewers: Toon Verwaest,

Description:
Do not externalize single-character strings.


[email protected]
BUG=v8:3480

Please review this at https://codereview.chromium.org/462643002/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files (+31, -10 lines):
  M src/api.cc
  M src/factory.cc
  M src/objects.cc
  M test/cctest/test-api.cc


Index: src/api.cc
diff --git a/src/api.cc b/src/api.cc

index7098859de3cfa9fa85d786b29cf8eef56846f086..25c620ad94aa30593cc154a25bd9de7713492886100644

--- a/src/api.cc
+++ b/src/api.cc
@@ -5475,7 +5475,7 @@ Local<String> v8::String::NewExternal(

bool v8::String::MakeExternal(v8::String::ExternalStringResource*resource) {

   i::Handle<i::String> obj = Utils::OpenHandle(this);
   i::Isolate* isolate = obj->GetIsolate();
-  if (i::StringShape(*obj).IsExternalTwoByte()) {
+  if (i::StringShape(*obj).IsExternal()) {
     return false;  // Already an external string.
   }
   ENTER_V8(isolate);

@@ -5488,6 +5488,8 @@ boolv8::String::MakeExternal(v8::String::ExternalStringResource* resource) {

   CHECK(resource && resource->data());

   bool result = obj->MakeExternal(resource);

+ // Assert that if CanMakeExternal(), then externalizing actuallysucceeds.

+  DCHECK(!CanMakeExternal() || result);
   if (result) {
     DCHECK(obj->IsExternalString());
     isolate->heap()->external_string_table()->AddString(*obj);
@@ -5515,7 +5517,7 @@ bool v8::String::MakeExternal(
     v8::String::ExternalAsciiStringResource* resource) {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
   i::Isolate* isolate = obj->GetIsolate();
-  if (i::StringShape(*obj).IsExternalTwoByte()) {
+  if (i::StringShape(*obj).IsExternal()) {
     return false;  // Already an external string.
   }
   ENTER_V8(isolate);
@@ -5528,6 +5530,8 @@ bool v8::String::MakeExternal(
   CHECK(resource && resource->data());

   bool result = obj->MakeExternal(resource);

+ // Assert that if CanMakeExternal(), then externalizing actuallysucceeds.

+  DCHECK(!CanMakeExternal() || result);
   if (result) {
     DCHECK(obj->IsExternalString());
     isolate->heap()->external_string_table()->AddString(*obj);
@@ -5549,6 +5553,8 @@ bool v8::String::CanMakeExternal() {
   if (isolate->string_tracker()->IsFreshUnusedString(obj)) return false;
   int size = obj->Size();  // Byte size of the original string.
   if (size < i::ExternalString::kShortSize) return false;
+  // Avoid externalizing entries of the single-character string cache.
+  if (obj->length() == 1) return false;
   i::StringShape shape(*obj);
   return !shape.IsExternal();
 }
Index: src/factory.cc
diff --git a/src/factory.cc b/src/factory.cc

indexa52b95719db05276362f7325ec32be751b37d87a..0d885fc3dbc851d8ce194e4ae8e6be62ce5b5076100644

--- a/src/factory.cc
+++ b/src/factory.cc

@@ -270,6 +270,7 @@ MaybeHandle<String>Factory::NewStringFromTwoByte(Vector<const uc16> string,

   int length = string.length();
   const uc16* start = string.start();
   if (String::IsOneByte(start, length)) {
+    if (length == 1) return LookupSingleCharacterStringFromCode(string[0]);
     Handle<SeqOneByteString> result;
     ASSIGN_RETURN_ON_EXCEPTION(
         isolate(),
Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc

index645bb4a74e39f81272c0c9e3c4c4d913976224fc..69e23078f823a9eaf56e83d89ea075275b9988e0100644

--- a/src/objects.cc
+++ b/src/objects.cc

@@ -1052,11 +1052,12 @@ boolString::MakeExternal(v8::String::ExternalStringResource* resource) {

                   resource->length() * sizeof(smart_chars[0])) == 0);
   }
 #endif  // DEBUG
-  Heap* heap = GetHeap();
+  // Avoid externalizing entries of the single character string cache.
+  if (this->length() == 1) return false;
   int size = this->Size();  // Byte size of the original string.
-  if (size < ExternalString::kShortSize) {
-    return false;
-  }
+  // Abort if size does not allow in-place conversion.
+  if (size < ExternalString::kShortSize) return false;
+  Heap* heap = GetHeap();
   bool is_ascii = this->IsOneByteRepresentation();
   bool is_internalized = this->IsInternalizedString();

@@ -1109,6 +1110,9 @@ boolString::MakeExternal(v8::String::ExternalStringResource* resource) {

bool String::MakeExternal(v8::String::ExternalAsciiStringResource*resource) {

+  // Externalizing twice leaks the external resource, so it's
+  // prohibited by the API.
+  DCHECK(!this->IsExternalString());
 #ifdef ENABLE_SLOW_DCHECKS
   if (FLAG_enable_slow_asserts) {
     // Assert that the resource and the string are equivalent.

@@ -1125,11 +1129,12 @@ boolString::MakeExternal(v8::String::ExternalAsciiStringResource* resource) {

                   resource->length() * sizeof(smart_chars[0])) == 0);
   }
 #endif  // DEBUG
-  Heap* heap = GetHeap();
+  // Avoid externalizing entries of the single character string cache.
+  if (this->length() == 1) return false;
   int size = this->Size();  // Byte size of the original string.
-  if (size < ExternalString::kShortSize) {
-    return false;
-  }
+  // Abort if size does not allow in-place conversion.
+  if (size < ExternalString::kShortSize) return false;
+  Heap* heap = GetHeap();
   bool is_internalized = this->IsInternalizedString();

   // Morph the string to an external string by replacing the map and
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc

index45c5d467d3143420408f858c91e6baa7e0d304ea..0a87725976a801d198491c71846b9db764e9f566100644

--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -589,9 +589,13 @@ TEST(MakingExternalStringConditions) {
   CcTest::heap()->CollectGarbage(i::NEW_SPACE);

   uint16_t* two_byte_string = AsciiToTwoByteString("s1");
+  uint16_t* single_char_string = AsciiToTwoByteString("s");
   Local<String> small_string =
       String::NewFromTwoByte(env->GetIsolate(), two_byte_string);
+  Local<String> single_char =
+      String::NewFromTwoByte(env->GetIsolate(), single_char_string);
   i::DeleteArray(two_byte_string);
+  i::DeleteArray(single_char_string);

   // We should refuse to externalize newly created small string.
   CHECK(!small_string->CanMakeExternal());
@@ -600,6 +604,8 @@ TEST(MakingExternalStringConditions) {
   CcTest::heap()->CollectGarbage(i::NEW_SPACE);  // in old gen now
   // Old space strings should be accepted.
   CHECK(small_string->CanMakeExternal());
+  // Single-character strings should never be accepted.
+  CHECK(!single_char->CanMakeExternal());

   two_byte_string = AsciiToTwoByteString("small string 2");

small_string = String::NewFromTwoByte(env->GetIsolate(),two_byte_string);

@@ -637,6 +643,7 @@ TEST(MakingExternalAsciiStringConditions) {
   CcTest::heap()->CollectGarbage(i::NEW_SPACE);

Local<String> small_string =String::NewFromUtf8(env->GetIsolate(), "s1");

+  Local<String> single_char = String::NewFromUtf8(env->GetIsolate(), "s");
   // We should refuse to externalize newly created small string.
   CHECK(!small_string->CanMakeExternal());
   // Trigger GCs so that the newly allocated string moves to old gen.
@@ -644,6 +651,8 @@ TEST(MakingExternalAsciiStringConditions) {
   CcTest::heap()->CollectGarbage(i::NEW_SPACE);  // in old gen now
   // Old space strings should be accepted.
   CHECK(small_string->CanMakeExternal());
+  // Single-character strings should never be accepted.
+  CHECK(!single_char->CanMakeExternal());

   small_string = String::NewFromUtf8(env->GetIsolate(), "small string 2");
   // We should refuse externalizing newly created small string.


--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

---You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Do not externalize single-character strings. (issue 462643002 by [email protected])

Reply via email to