From da9386ae2d0416ac883cd1ce343a0ca3eac43519 Mon Sep 17 00:00:00 2001 From: Rodrigo Bruno Date: Wed, 25 Jul 2018 11:53:08 +0200 Subject: [PATCH] Reland^2 "Avoiding re-externalization of strings" Previously landed as 2c4c2ad694dfd4e852039644c7bfe22e594587c6 / #54599 and f34158c9d2abec367ec3930732fdf294cd1ca188 / #54637 Previously reviewed at https://chromium-review.googlesource.com/1139056 and https://chromium-review.googlesource.com/1146583 Bug: chromium:845409, chromium:866208 Change-Id: Idb1b6d1b29499f66bf8cd704977c40b027f99dbd Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Reviewed-on: https://chromium-review.googlesource.com/1148281 Reviewed-by: Toon Verwaest Reviewed-by: Michael Lippautz Reviewed-by: Yang Guo Reviewed-by: Jakob Kummerow Reviewed-by: Dan Elphick Commit-Queue: Rodrigo Bruno Cr-Commit-Position: refs/heads/master@{#54703} --- include/v8.h | 17 +- src/api.cc | 178 ++++++++++++------ .../externalize-string-extension.cc | 11 +- src/heap/heap-inl.h | 2 + src/heap/heap.cc | 10 + src/heap/heap.h | 1 + src/objects.cc | 23 ++- src/objects/string.h | 1 + 8 files changed, 176 insertions(+), 67 deletions(-) diff --git a/include/v8.h b/include/v8.h index d1d146454c..51dd766660 100644 --- a/include/v8.h +++ b/include/v8.h @@ -3053,6 +3053,12 @@ class V8_EXPORT String : public Name { void VerifyExternalStringResourceBase(ExternalStringResourceBase* v, Encoding encoding) const; void VerifyExternalStringResource(ExternalStringResource* val) const; + ExternalStringResource* GetExternalStringResourceSlow() const; + ExternalStringResourceBase* GetExternalStringResourceBaseSlow( + String::Encoding* encoding_out) const; + const ExternalOneByteStringResource* GetExternalOneByteStringResourceSlow() + const; + static void CheckCast(v8::Value* obj); }; @@ -10187,12 +10193,13 @@ String::ExternalStringResource* String::GetExternalStringResource() const { typedef internal::Object O; typedef internal::Internals I; O* obj = *reinterpret_cast(this); - String::ExternalStringResource* result; + + ExternalStringResource* result; if (I::IsExternalTwoByteString(I::GetInstanceType(obj))) { void* value = I::ReadField(obj, I::kStringResourceOffset); result = reinterpret_cast(value); } else { - result = NULL; + result = GetExternalStringResourceSlow(); } #ifdef V8_ENABLE_CHECKS VerifyExternalStringResource(result); @@ -10208,14 +10215,16 @@ String::ExternalStringResourceBase* String::GetExternalStringResourceBase( O* obj = *reinterpret_cast(this); int type = I::GetInstanceType(obj) & I::kFullStringRepresentationMask; *encoding_out = static_cast(type & I::kStringEncodingMask); - ExternalStringResourceBase* resource = NULL; + ExternalStringResourceBase* resource; if (type == I::kExternalOneByteRepresentationTag || type == I::kExternalTwoByteRepresentationTag) { void* value = I::ReadField(obj, I::kStringResourceOffset); resource = static_cast(value); + } else { + resource = GetExternalStringResourceBaseSlow(encoding_out); } #ifdef V8_ENABLE_CHECKS - VerifyExternalStringResourceBase(resource, *encoding_out); + VerifyExternalStringResourceBase(resource, *encoding_out); #endif return resource; } diff --git a/src/api.cc b/src/api.cc index b312a1c84f..89090657f9 100644 --- a/src/api.cc +++ b/src/api.cc @@ -5932,11 +5932,16 @@ bool v8::String::IsExternalOneByte() const { void v8::String::VerifyExternalStringResource( v8::String::ExternalStringResource* value) const { - i::Handle str = Utils::OpenHandle(this); + i::DisallowHeapAllocation no_allocation; + i::String* str = *Utils::OpenHandle(this); const v8::String::ExternalStringResource* expected; - if (i::StringShape(*str).IsExternalTwoByte()) { - const void* resource = - i::Handle::cast(str)->resource(); + + if (str->IsThinString()) { + str = i::ThinString::cast(str)->actual(); + } + + if (i::StringShape(str).IsExternalTwoByte()) { + const void* resource = i::ExternalTwoByteString::cast(str)->resource(); expected = reinterpret_cast(resource); } else { expected = nullptr; @@ -5946,17 +5951,21 @@ void v8::String::VerifyExternalStringResource( void v8::String::VerifyExternalStringResourceBase( v8::String::ExternalStringResourceBase* value, Encoding encoding) const { - i::Handle str = Utils::OpenHandle(this); + i::DisallowHeapAllocation no_allocation; + i::String* str = *Utils::OpenHandle(this); const v8::String::ExternalStringResourceBase* expected; Encoding expectedEncoding; - if (i::StringShape(*str).IsExternalOneByte()) { - const void* resource = - i::Handle::cast(str)->resource(); + + if (str->IsThinString()) { + str = i::ThinString::cast(str)->actual(); + } + + if (i::StringShape(str).IsExternalOneByte()) { + const void* resource = i::ExternalOneByteString::cast(str)->resource(); expected = reinterpret_cast(resource); expectedEncoding = ONE_BYTE_ENCODING; - } else if (i::StringShape(*str).IsExternalTwoByte()) { - const void* resource = - i::Handle::cast(str)->resource(); + } else if (i::StringShape(str).IsExternalTwoByte()) { + const void* resource = i::ExternalTwoByteString::cast(str)->resource(); expected = reinterpret_cast(resource); expectedEncoding = TWO_BYTE_ENCODING; } else { @@ -5968,15 +5977,69 @@ void v8::String::VerifyExternalStringResourceBase( CHECK_EQ(expectedEncoding, encoding); } +String::ExternalStringResource* String::GetExternalStringResourceSlow() const { + i::DisallowHeapAllocation no_allocation; + typedef internal::Internals I; + ExternalStringResource* result = nullptr; + i::String* str = *Utils::OpenHandle(this); + + if (str->IsThinString()) { + str = i::ThinString::cast(str)->actual(); + } + + if (i::StringShape(str).IsExternalTwoByte()) { + void* value = I::ReadField(str, I::kStringResourceOffset); + result = reinterpret_cast(value); + } + return result; +} + +String::ExternalStringResourceBase* String::GetExternalStringResourceBaseSlow( + String::Encoding* encoding_out) const { + i::DisallowHeapAllocation no_allocation; + typedef internal::Internals I; + ExternalStringResourceBase* resource = nullptr; + i::String* str = *Utils::OpenHandle(this); + + if (str->IsThinString()) { + str = i::ThinString::cast(str)->actual(); + } + + int type = I::GetInstanceType(str) & I::kFullStringRepresentationMask; + *encoding_out = static_cast(type & I::kStringEncodingMask); + if (i::StringShape(str).IsExternalOneByte() || + i::StringShape(str).IsExternalTwoByte()) { + void* value = I::ReadField(str, I::kStringResourceOffset); + resource = static_cast(value); + } + return resource; +} + +const String::ExternalOneByteStringResource* +String::GetExternalOneByteStringResourceSlow() const { + i::DisallowHeapAllocation no_allocation; + i::String* str = *Utils::OpenHandle(this); + + if (str->IsThinString()) { + str = i::ThinString::cast(str)->actual(); + } + + if (i::StringShape(str).IsExternalOneByte()) { + const void* resource = i::ExternalOneByteString::cast(str)->resource(); + return reinterpret_cast(resource); + } + return nullptr; +} + const v8::String::ExternalOneByteStringResource* v8::String::GetExternalOneByteStringResource() const { - i::Handle str = Utils::OpenHandle(this); - if (i::StringShape(*str).IsExternalOneByte()) { - const void* resource = - i::Handle::cast(str)->resource(); + i::DisallowHeapAllocation no_allocation; + i::String* str = *Utils::OpenHandle(this); + if (i::StringShape(str).IsExternalOneByte()) { + const void* resource = i::ExternalOneByteString::cast(str)->resource(); return reinterpret_cast(resource); } else { - return nullptr; + return GetExternalOneByteStringResourceSlow(); } } @@ -6863,73 +6926,76 @@ Local v8::String::NewExternal( bool v8::String::MakeExternal(v8::String::ExternalStringResource* resource) { - i::Handle obj = Utils::OpenHandle(this); + i::DisallowHeapAllocation no_allocation; + i::String* obj = *Utils::OpenHandle(this); + + if (obj->IsThinString()) { + obj = i::ThinString::cast(obj)->actual(); + } + + if (!obj->SupportsExternalization()) { + return false; + } + + // It is safe to call FromWritable because SupportsExternalization already + // checked that the object is writable. i::Isolate* isolate; - if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) { - // RO_SPACE strings cannot be externalized. - return false; - } - - if (i::StringShape(*obj).IsExternal()) { - return false; // Already an external string. - } + i::Isolate::FromWritableHeapObject(obj, &isolate); ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); - if (isolate->heap()->IsInGCPostProcessing()) { - return false; - } + CHECK(resource && resource->data()); bool result = obj->MakeExternal(resource); - // Assert that if CanMakeExternal(), then externalizing actually succeeds. - DCHECK(!CanMakeExternal() || result); - if (result) { - DCHECK(obj->IsExternalString()); - } + DCHECK(result); + DCHECK(obj->IsExternalString()); return result; } bool v8::String::MakeExternal( v8::String::ExternalOneByteStringResource* resource) { - i::Handle obj = Utils::OpenHandle(this); + i::DisallowHeapAllocation no_allocation; + i::String* obj = *Utils::OpenHandle(this); + + if (obj->IsThinString()) { + obj = i::ThinString::cast(obj)->actual(); + } + + if (!obj->SupportsExternalization()) { + return false; + } + + // It is safe to call FromWritable because SupportsExternalization already + // checked that the object is writable. i::Isolate* isolate; - if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) { - // RO_SPACE strings cannot be externalized. - return false; - } - - if (i::StringShape(*obj).IsExternal()) { - return false; // Already an external string. - } + i::Isolate::FromWritableHeapObject(obj, &isolate); ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate); - if (isolate->heap()->IsInGCPostProcessing()) { - return false; - } + CHECK(resource && resource->data()); bool result = obj->MakeExternal(resource); - // Assert that if CanMakeExternal(), then externalizing actually succeeds. - DCHECK(!CanMakeExternal() || result); - if (result) { - DCHECK(obj->IsExternalString()); - } + DCHECK(result); + DCHECK(obj->IsExternalString()); return result; } bool v8::String::CanMakeExternal() { - i::Handle obj = Utils::OpenHandle(this); - if (obj->IsExternalString()) return false; + i::DisallowHeapAllocation no_allocation; + i::String* obj = *Utils::OpenHandle(this); - i::Isolate* isolate; - if (!i::Isolate::FromWritableHeapObject(*obj, &isolate)) { - // RO_SPACE strings cannot be externalized. + if (obj->IsThinString()) { + obj = i::ThinString::cast(obj)->actual(); + } + + if (!obj->SupportsExternalization()) { return false; } + // Only old space strings should be externalized. - return !i::Heap::InNewSpace(*obj); + return !i::Heap::InNewSpace(obj); } diff --git a/src/extensions/externalize-string-extension.cc b/src/extensions/externalize-string-extension.cc index 32c9cf7de4..de1530ba27 100644 --- a/src/extensions/externalize-string-extension.cc +++ b/src/extensions/externalize-string-extension.cc @@ -85,11 +85,12 @@ void ExternalizeStringExtension::Externalize( } bool result = false; Handle string = Utils::OpenHandle(*args[0].As()); - if (string->IsExternalString()) { + if (!string->SupportsExternalization()) { args.GetIsolate()->ThrowException( v8::String::NewFromUtf8(args.GetIsolate(), - "externalizeString() can't externalize twice.", - NewStringType::kNormal).ToLocalChecked()); + "string does not support externalization.", + NewStringType::kNormal) + .ToLocalChecked()); return; } if (string->IsOneByteRepresentation() && !force_two_byte) { @@ -97,14 +98,14 @@ void ExternalizeStringExtension::Externalize( String::WriteToFlat(*string, data, 0, string->length()); SimpleOneByteStringResource* resource = new SimpleOneByteStringResource( reinterpret_cast(data), string->length()); - result = string->MakeExternal(resource); + result = Utils::ToLocal(string)->MakeExternal(resource); if (!result) delete resource; } else { uc16* data = new uc16[string->length()]; String::WriteToFlat(*string, data, 0, string->length()); SimpleTwoByteStringResource* resource = new SimpleTwoByteStringResource( data, string->length()); - result = string->MakeExternal(resource); + result = Utils::ToLocal(string)->MakeExternal(resource); if (!result) delete resource; } if (!result) { diff --git a/src/heap/heap-inl.h b/src/heap/heap-inl.h index 5ad1a1bdd6..02a7dc985a 100644 --- a/src/heap/heap-inl.h +++ b/src/heap/heap-inl.h @@ -530,6 +530,8 @@ Isolate* Heap::isolate() { void Heap::ExternalStringTable::AddString(String* string) { DCHECK(string->IsExternalString()); + DCHECK(!Contains(string)); + if (InNewSpace(string)) { new_space_strings_.push_back(string); } else { diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 9b6d15a471..1daad8db21 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -2275,6 +2275,16 @@ void Heap::ProtectUnprotectedMemoryChunks() { unprotected_memory_chunks_.clear(); } +bool Heap::ExternalStringTable::Contains(HeapObject* obj) { + for (size_t i = 0; i < new_space_strings_.size(); ++i) { + if (new_space_strings_[i] == obj) return true; + } + for (size_t i = 0; i < old_space_strings_.size(); ++i) { + if (old_space_strings_[i] == obj) return true; + } + return false; +} + String* Heap::UpdateNewSpaceReferenceInExternalStringTableEntry(Heap* heap, Object** p) { MapWord first_word = HeapObject::cast(*p)->map_word(); diff --git a/src/heap/heap.h b/src/heap/heap.h index 2ab561e193..7735555000 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -1481,6 +1481,7 @@ class Heap { // Registers an external string. inline void AddString(String* string); + bool Contains(HeapObject* obj); void IterateAll(RootVisitor* v); void IterateNewSpaceStrings(RootVisitor* v); diff --git a/src/objects.cc b/src/objects.cc index ac2698d2bd..082f2bb3ba 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -2576,7 +2576,7 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) { DisallowHeapAllocation no_allocation; // Externalizing twice leaks the external resource, so it's // prohibited by the API. - DCHECK(!this->IsExternalString()); + DCHECK(this->SupportsExternalization()); DCHECK(!resource->IsCompressible()); #ifdef ENABLE_SLOW_DCHECKS if (FLAG_enable_slow_asserts) { @@ -2656,7 +2656,7 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) { DisallowHeapAllocation no_allocation; // Externalizing twice leaks the external resource, so it's // prohibited by the API. - DCHECK(!this->IsExternalString()); + DCHECK(this->SupportsExternalization()); DCHECK(!resource->IsCompressible()); #ifdef ENABLE_SLOW_DCHECKS if (FLAG_enable_slow_asserts) { @@ -2725,6 +2725,25 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) { return true; } +bool String::SupportsExternalization() { + if (this->IsThinString()) { + return i::ThinString::cast(this)->actual()->SupportsExternalization(); + } + + Isolate* isolate; + // RO_SPACE strings cannot be externalized. + if (!Isolate::FromWritableHeapObject(this, &isolate)) { + return false; + } + + // Already an external string. + if (StringShape(this).IsExternal()) { + return false; + } + + return !isolate->heap()->IsInGCPostProcessing(); +} + void String::StringShortPrint(StringStream* accumulator, bool show_details) { int len = length(); if (len > kMaxShortPrintLength) { diff --git a/src/objects/string.h b/src/objects/string.h index ba036edee9..47543041c4 100644 --- a/src/objects/string.h +++ b/src/objects/string.h @@ -311,6 +311,7 @@ class String : public Name { // Externalization. bool MakeExternal(v8::String::ExternalStringResource* resource); bool MakeExternal(v8::String::ExternalOneByteStringResource* resource); + bool SupportsExternalization(); // Conversion. inline bool AsArrayIndex(uint32_t* index);