[V8] Make String::CanMakeExternal ignore the length of new strings

src/api.cc

Index: src/api.cc
diff --git a/src/api.cc b/src/api.cc
index 42c59538337a39db3796c733a51dcd2c23a693b9..cb5811810f4e80ed48ef4e20185a4d69c93080f9 100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -6011,12 +6011,8 @@ bool v8::String::CanMakeExternal() {
   i::Handle<i::String> obj = Utils::OpenHandle(this);
   i::Isolate* isolate = obj->GetIsolate();
 
-  // Old space strings should be externalized.
-  if (!isolate->heap()->new_space()->Contains(*obj)) return true;
-  int size = obj->Size();  // Byte size of the original string.
-  if (size <= i::ExternalString::kShortSize) return false;
-  i::StringShape shape(*obj);
-  return !shape.IsExternal();

[Yang, 2016/06/09 05:45:05]

Why drop this condition? If the string is already external, we would copy out
the content, and call MakeExternal only for it to return false because there is
nothing to be done.

[peria, 2016/06/09 06:51:59]

I agree that externalized strings should not be externalized again.  However, if
we keep it as-is, every string is externalized.

I think it should be placed just after getting |obj|, if creating a StringShape
and IsExternal() are cheaper than NewSpace::Contains().
(BTW, why don't we use Value::IsExternal() here?)

+  // Do not externalize new strings.
+  return !isolate->heap()->new_space()->Contains(*obj);
 }