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

src/api.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/api.h"
 
 #include <string.h>  // For memcpy, strlen.
 #ifdef V8_USE_ADDRESS_SANITIZER
 #include <sanitizer/asan_interface.h>
 #endif  // V8_USE_ADDRESS_SANITIZER
@@ skipping 5993 matching lines @@
     isolate->heap()->RegisterExternalString(*obj);
   }
   return result;
 }
 
 
 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);
 }
 
 
 Isolate* v8::Object::GetIsolate() {
   i::Isolate* i_isolate = Utils::OpenHandle(this)->GetIsolate();
   return reinterpret_cast<Isolate*>(i_isolate);
 }
 
 
 Local<v8::Object> v8::Object::New(Isolate* isolate) {
@@ skipping 2794 matching lines @@
   Address callback_address =
       reinterpret_cast<Address>(reinterpret_cast<intptr_t>(callback));
   VMState<EXTERNAL> state(isolate);
   ExternalCallbackScope call_scope(isolate, callback_address);
   callback(info);
 }
 
 
 }  // namespace internal
 }  // namespace v8