Skip to content

Commit bfe3d6b

Browse files
gahaasCommit Bot
gahaas
authored and
Commit Bot
committed
[api] Deprecate [Shared]ArrayBuffer::Externalize/GetContents and constructors
The new API with v8::BackingStore should be used instead as explained in https://docs.google.com/document/d/1sTc_jRL87Fu175Holm5SV0kajkseGl2r8ifGY76G35k This also relaxes the pre-condition for [Shared]ArrayBuffer::Detach to not require externalization first. Bug: v8:9380, v8:9908 Change-Id: Idd119fcd28be84a2fae74ae86f7381fd997766f5 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1859628 Commit-Queue: Andreas Haas <[email protected]> Reviewed-by: Clemens Backes <[email protected]> Reviewed-by: Andreas Haas <[email protected]> Cr-Commit-Position: refs/heads/master@{#64625}
1 parent 30ccfb2 commit bfe3d6b

16 files changed

Lines changed: 120 additions & 142 deletions

‎BUILD.gn‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2185,8 +2185,6 @@ v8_source_set("v8_base_without_compiler") {
21852185
"src/extensions/cputracemark-extension.h",
21862186
"src/extensions/externalize-string-extension.cc",
21872187
"src/extensions/externalize-string-extension.h",
2188-
"src/extensions/free-buffer-extension.cc",
2189-
"src/extensions/free-buffer-extension.h",
21902188
"src/extensions/gc-extension.cc",
21912189
"src/extensions/gc-extension.h",
21922190
"src/extensions/ignition-statistics-extension.cc",

‎include/v8.h‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4846,6 +4846,7 @@ enum class ArrayBufferCreationMode { kInternalized, kExternalized };
48464846

48474847
/**
48484848
* A wrapper around the backing store (i.e. the raw memory) of an array buffer.
4849+
* See a document linked in http://crbug.com/v8/9908 for more information.
48494850
*
48504851
* The allocation and destruction of backing stores is generally managed by
48514852
* V8. Clients should always use standard C++ memory ownership types (i.e.
@@ -4879,6 +4880,10 @@ class V8_EXPORT BackingStore : public v8::internal::BackingStoreBase {
48794880
bool IsShared() const;
48804881

48814882
private:
4883+
/**
4884+
* See [Shared]ArrayBuffer::GetBackingStore and
4885+
* [Shared]ArrayBuffer::NewBackingStore.
4886+
*/
48824887
BackingStore();
48834888
};
48844889

@@ -5021,6 +5026,9 @@ class V8_EXPORT ArrayBuffer : public Object {
50215026
* |Allocator::Free| once all ArrayBuffers referencing it are collected by
50225027
* the garbage collector.
50235028
*/
5029+
V8_DEPRECATE_SOON(
5030+
"Use the version that takes a BackingStore. "
5031+
"See http://crbug.com/v8/9908.")
50245032
static Local<ArrayBuffer> New(
50255033
Isolate* isolate, void* data, size_t byte_length,
50265034
ArrayBufferCreationMode mode = ArrayBufferCreationMode::kExternalized);
@@ -5067,6 +5075,9 @@ class V8_EXPORT ArrayBuffer : public Object {
50675075
* Returns true if ArrayBuffer is externalized, that is, does not
50685076
* own its memory block.
50695077
*/
5078+
V8_DEPRECATE_SOON(
5079+
"With v8::BackingStore externalized ArrayBuffers are "
5080+
"the same as ordinary ArrayBuffers. See http://crbug.com/v8/9908.")
50705081
bool IsExternal() const;
50715082

50725083
/**
@@ -5092,6 +5103,8 @@ class V8_EXPORT ArrayBuffer : public Object {
50925103
* deleter, which will call ArrayBuffer::Allocator::Free if the buffer
50935104
* was allocated with ArrayBuffer::Allocator::Allocate.
50945105
*/
5106+
V8_DEPRECATE_SOON(
5107+
"Use GetBackingStore or Detach. See http://crbug.com/v8/9908.")
50955108
Contents Externalize();
50965109

50975110
/**
@@ -5101,6 +5114,7 @@ class V8_EXPORT ArrayBuffer : public Object {
51015114
* With the new lifetime management of backing stores there is no need for
51025115
* externalizing, so this function exists only to make the transition easier.
51035116
*/
5117+
V8_DEPRECATE_SOON("This will be removed together with IsExternal.")
51045118
void Externalize(const std::shared_ptr<BackingStore>& backing_store);
51055119

51065120
/**
@@ -5111,6 +5125,7 @@ class V8_EXPORT ArrayBuffer : public Object {
51115125
* The embedder should make sure to hold a strong reference to the
51125126
* ArrayBuffer while accessing this pointer.
51135127
*/
5128+
V8_DEPRECATE_SOON("Use GetBackingStore. See http://crbug.com/v8/9908.")
51145129
Contents GetContents();
51155130

51165131
/**
@@ -5494,6 +5509,9 @@ class V8_EXPORT SharedArrayBuffer : public Object {
54945509
* specified. The memory block will not be reclaimed when a created
54955510
* SharedArrayBuffer is garbage-collected.
54965511
*/
5512+
V8_DEPRECATE_SOON(
5513+
"Use the version that takes a BackingStore. "
5514+
"See http://crbug.com/v8/9908.")
54975515
static Local<SharedArrayBuffer> New(
54985516
Isolate* isolate, void* data, size_t byte_length,
54995517
ArrayBufferCreationMode mode = ArrayBufferCreationMode::kExternalized);
@@ -5540,7 +5558,9 @@ class V8_EXPORT SharedArrayBuffer : public Object {
55405558
* Create a new SharedArrayBuffer over an existing memory block. Propagate
55415559
* flags to indicate whether the underlying buffer can be grown.
55425560
*/
5543-
V8_DEPRECATED("Use New method with data, and byte_length instead.")
5561+
V8_DEPRECATED(
5562+
"Use the version that takes a BackingStore. "
5563+
"See http://crbug.com/v8/9908.")
55445564
static Local<SharedArrayBuffer> New(
55455565
Isolate* isolate, const SharedArrayBuffer::Contents&,
55465566
ArrayBufferCreationMode mode = ArrayBufferCreationMode::kExternalized);
@@ -5549,6 +5569,9 @@ class V8_EXPORT SharedArrayBuffer : public Object {
55495569
* Returns true if SharedArrayBuffer is externalized, that is, does not
55505570
* own its memory block.
55515571
*/
5572+
V8_DEPRECATE_SOON(
5573+
"With v8::BackingStore externalized SharedArrayBuffers are the same "
5574+
"as ordinary SharedArrayBuffers. See http://crbug.com/v8/9908.")
55525575
bool IsExternal() const;
55535576

55545577
/**
@@ -5563,6 +5586,8 @@ class V8_EXPORT SharedArrayBuffer : public Object {
55635586
* v8::Isolate::CreateParams::array_buffer_allocator.
55645587
*
55655588
*/
5589+
V8_DEPRECATE_SOON(
5590+
"Use GetBackingStore or Detach. See http://crbug.com/v8/9908.")
55665591
Contents Externalize();
55675592

55685593
/**
@@ -5572,6 +5597,7 @@ class V8_EXPORT SharedArrayBuffer : public Object {
55725597
* With the new lifetime management of backing stores there is no need for
55735598
* externalizing, so this function exists only to make the transition easier.
55745599
*/
5600+
V8_DEPRECATE_SOON("This will be removed together with IsExternal.")
55755601
void Externalize(const std::shared_ptr<BackingStore>& backing_store);
55765602

55775603
/**
@@ -5586,6 +5612,7 @@ class V8_EXPORT SharedArrayBuffer : public Object {
55865612
* by the allocator specified in
55875613
* v8::Isolate::CreateParams::array_buffer_allocator.
55885614
*/
5615+
V8_DEPRECATE_SOON("Use GetBackingStore. See http://crbug.com/v8/9908.")
55895616
Contents GetContents();
55905617

55915618
/**

‎src/api/api.cc‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7433,8 +7433,6 @@ v8::ArrayBuffer::Contents v8::ArrayBuffer::GetContents(bool externalize) {
74337433
void v8::ArrayBuffer::Detach() {
74347434
i::Handle<i::JSArrayBuffer> obj = Utils::OpenHandle(this);
74357435
i::Isolate* isolate = obj->GetIsolate();
7436-
Utils::ApiCheck(obj->is_external(), "v8::ArrayBuffer::Detach",
7437-
"Only externalized ArrayBuffers can be detached");
74387436
Utils::ApiCheck(obj->is_detachable(), "v8::ArrayBuffer::Detach",
74397437
"Only detachable ArrayBuffers can be detached");
74407438
LOG_API(isolate, ArrayBuffer, Detach);

‎src/d8/d8.cc‎

Lines changed: 11 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2317,23 +2317,6 @@ static char* ReadChars(const char* name, int* size_out) {
23172317
return chars;
23182318
}
23192319

2320-
struct DataAndPersistent {
2321-
uint8_t* data;
2322-
int byte_length;
2323-
Global<ArrayBuffer> handle;
2324-
};
2325-
2326-
static void ReadBufferWeakCallback(
2327-
const v8::WeakCallbackInfo<DataAndPersistent>& data) {
2328-
int byte_length = data.GetParameter()->byte_length;
2329-
data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(
2330-
-static_cast<intptr_t>(byte_length));
2331-
2332-
delete[] data.GetParameter()->data;
2333-
data.GetParameter()->handle.Reset();
2334-
delete data.GetParameter();
2335-
}
2336-
23372320
void Shell::ReadBuffer(const v8::FunctionCallbackInfo<v8::Value>& args) {
23382321
static_assert(sizeof(char) == sizeof(uint8_t),
23392322
"char and uint8_t should both have 1 byte");
@@ -2345,19 +2328,20 @@ void Shell::ReadBuffer(const v8::FunctionCallbackInfo<v8::Value>& args) {
23452328
return;
23462329
}
23472330

2348-
DataAndPersistent* data = new DataAndPersistent;
2349-
data->data = reinterpret_cast<uint8_t*>(ReadChars(*filename, &length));
2350-
if (data->data == nullptr) {
2351-
delete data;
2331+
uint8_t* data = reinterpret_cast<uint8_t*>(ReadChars(*filename, &length));
2332+
if (data == nullptr) {
23522333
Throw(isolate, "Error reading file");
23532334
return;
23542335
}
2355-
data->byte_length = length;
2356-
Local<v8::ArrayBuffer> buffer = ArrayBuffer::New(isolate, data->data, length);
2357-
data->handle.Reset(isolate, buffer);
2358-
data->handle.SetWeak(data, ReadBufferWeakCallback,
2359-
v8::WeakCallbackType::kParameter);
2360-
isolate->AdjustAmountOfExternalAllocatedMemory(length);
2336+
std::unique_ptr<v8::BackingStore> backing_store =
2337+
ArrayBuffer::NewBackingStore(
2338+
data, length,
2339+
[](void* data, size_t length, void*) {
2340+
delete[] reinterpret_cast<uint8_t*>(data);
2341+
},
2342+
nullptr);
2343+
Local<v8::ArrayBuffer> buffer =
2344+
ArrayBuffer::New(isolate, std::move(backing_store));
23612345

23622346
args.GetReturnValue().Set(buffer);
23632347
}
@@ -3394,9 +3378,6 @@ class Serializer : public ValueSerializer::Delegate {
33943378
}
33953379

33963380
auto backing_store = array_buffer->GetBackingStore();
3397-
if (!array_buffer->IsExternal()) {
3398-
array_buffer->Externalize(backing_store);
3399-
}
34003381
data_->backing_stores_.push_back(std::move(backing_store));
34013382
array_buffer->Detach();
34023383
}

‎src/extensions/free-buffer-extension.cc‎

Lines changed: 0 additions & 29 deletions
This file was deleted.

‎src/extensions/free-buffer-extension.h‎

Lines changed: 0 additions & 25 deletions
This file was deleted.

‎src/flags/flag-definitions.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1038,7 +1038,6 @@ DEFINE_BOOL(disable_old_api_accessors, false,
10381038
"prototype chain")
10391039

10401040
// bootstrapper.cc
1041-
DEFINE_BOOL(expose_free_buffer, false, "expose freeBuffer extension")
10421041
DEFINE_BOOL(expose_gc, false, "expose gc extension")
10431042
DEFINE_STRING(expose_gc_as, nullptr,
10441043
"expose gc extension under the specified name")

‎src/init/bootstrapper.cc‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
#include "src/execution/protectors.h"
1616
#include "src/extensions/cputracemark-extension.h"
1717
#include "src/extensions/externalize-string-extension.h"
18-
#include "src/extensions/free-buffer-extension.h"
1918
#include "src/extensions/gc-extension.h"
2019
#include "src/extensions/ignition-statistics-extension.h"
2120
#include "src/extensions/statistics-extension.h"
@@ -121,7 +120,6 @@ static bool isValidCpuTraceMarkFunctionName() {
121120
}
122121

123122
void Bootstrapper::InitializeOncePerProcess() {
124-
v8::RegisterExtension(std::make_unique<FreeBufferExtension>());
125123
v8::RegisterExtension(std::make_unique<GCExtension>(GCFunctionName()));
126124
v8::RegisterExtension(std::make_unique<ExternalizeStringExtension>());
127125
v8::RegisterExtension(std::make_unique<StatisticsExtension>());
@@ -4958,8 +4956,6 @@ bool Genesis::InstallExtensions(Isolate* isolate,
49584956
v8::ExtensionConfiguration* extensions) {
49594957
ExtensionStates extension_states; // All extensions have state UNVISITED.
49604958
return InstallAutoExtensions(isolate, &extension_states) &&
4961-
(!FLAG_expose_free_buffer ||
4962-
InstallExtension(isolate, "v8/free-buffer", &extension_states)) &&
49634959
(!FLAG_expose_gc ||
49644960
InstallExtension(isolate, "v8/gc", &extension_states)) &&
49654961
(!FLAG_expose_externalize_string ||

‎test/cctest/heap/test-array-buffer-tracker.cc‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ TEST(ArrayBuffer_Compaction) {
151151
TEST(ArrayBuffer_UnregisterDuringSweep) {
152152
// Regular pages in old space (without compaction) are processed concurrently
153153
// in the sweeper. If we happen to unregister a buffer (either explicitly, or
154-
// implicitly through e.g. |Externalize|) we need to sync with the sweeper
154+
// implicitly through e.g. |Detach|) we need to sync with the sweeper
155155
// task.
156156
//
157157
// Note: This test will will only fail on TSAN configurations.
@@ -189,12 +189,10 @@ TEST(ArrayBuffer_UnregisterDuringSweep) {
189189
}
190190

191191
CcTest::CollectGarbage(OLD_SPACE);
192-
// |Externalize| will cause the buffer to be |Unregister|ed. Without
192+
// |Detach| will cause the buffer to be |Unregister|ed. Without
193193
// barriers and proper synchronization this will trigger a data race on
194194
// TSAN.
195-
v8::ArrayBuffer::Contents contents = ab->Externalize();
196-
contents.Deleter()(contents.Data(), contents.ByteLength(),
197-
contents.DeleterData());
195+
ab->Detach();
198196
}
199197
}
200198

‎test/cctest/manually-externalized-buffer.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,16 @@ namespace testing {
1515
// externalized in a test.
1616
struct ManuallyExternalizedBuffer {
1717
Handle<JSArrayBuffer> buffer_;
18-
v8::ArrayBuffer::Contents contents_;
18+
std::shared_ptr<v8::BackingStore> backing_store_;
1919

2020
explicit ManuallyExternalizedBuffer(Handle<JSArrayBuffer> buffer)
2121
: buffer_(buffer),
22-
contents_(v8::Utils::ToLocal(buffer_)->Externalize()) {}
22+
backing_store_(v8::Utils::ToLocal(buffer_)->GetBackingStore()) {}
2323
~ManuallyExternalizedBuffer() {
24-
contents_.Deleter()(contents_.Data(), contents_.ByteLength(),
25-
contents_.DeleterData());
24+
// Nothing to be done. The reference to the backing store will be
25+
// dropped automatically.
2626
}
27-
void* backing_store() { return contents_.Data(); }
27+
void* backing_store() { return backing_store_->Data(); }
2828
};
2929

3030
} // namespace testing

0 commit comments

Comments
 (0)