Skip to content

Commit fa8189f

Browse files
joyeecheungjoyeecheung
committed
src: make AliasedBuffers in the binding data weak
The binding data holds references to the AliasedBuffers directly from their wrappers which already ensures that the AliasedBuffers won't be accessed when the wrappers are GC'ed. So we can just make the global references to the AliasedBuffers weak. This way we can simply deserialize the typed arrays when deserialize the binding data and avoid the extra Object::Set() calls. It also eliminates the caveat in the JS land where aliased buffers must be dynamically read from the binding.
1 parent 39a08ee commit fa8189f

11 files changed

+220
-93
lines changed

‎lib/v8.js

+7-3
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,10 @@ const {
139139
kBytecodeAndMetadataSizeIndex,
140140
kExternalScriptSourceSizeIndex,
141141
kCPUProfilerMetaDataSizeIndex,
142+
143+
heapStatisticsBuffer,
144+
heapCodeStatisticsBuffer,
145+
heapSpaceStatisticsBuffer,
142146
} = binding;
143147

144148
const kNumberOfHeapSpaces = kHeapSpaces.length;
@@ -170,7 +174,7 @@ function setFlagsFromString(flags) {
170174
* }}
171175
*/
172176
function getHeapStatistics() {
173-
const buffer = binding.heapStatisticsBuffer;
177+
const buffer = heapStatisticsBuffer;
174178

175179
updateHeapStatisticsBuffer();
176180

@@ -204,7 +208,7 @@ function getHeapStatistics() {
204208
*/
205209
function getHeapSpaceStatistics() {
206210
const heapSpaceStatistics = new Array(kNumberOfHeapSpaces);
207-
const buffer = binding.heapSpaceStatisticsBuffer;
211+
const buffer = heapSpaceStatisticsBuffer;
208212

209213
for (let i = 0; i < kNumberOfHeapSpaces; i++) {
210214
updateHeapSpaceStatisticsBuffer(i);
@@ -230,7 +234,7 @@ function getHeapSpaceStatistics() {
230234
* }}
231235
*/
232236
function getHeapCodeStatistics() {
233-
const buffer = binding.heapCodeStatisticsBuffer;
237+
const buffer = heapCodeStatisticsBuffer;
234238

235239
updateHeapCodeStatisticsBuffer();
236240
return {

‎src/aliased_buffer-inl.h

+28-9
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,14 @@ AliasedBufferBase<NativeT, V8T>::AliasedBufferBase(
7070
count_(that.count_),
7171
byte_offset_(that.byte_offset_),
7272
buffer_(that.buffer_) {
73-
DCHECK_NULL(index_);
73+
DCHECK(is_valid());
7474
js_array_ = v8::Global<V8T>(that.isolate_, that.GetJSArray());
7575
}
7676

7777
template <typename NativeT, typename V8T>
7878
AliasedBufferIndex AliasedBufferBase<NativeT, V8T>::Serialize(
7979
v8::Local<v8::Context> context, v8::SnapshotCreator* creator) {
80-
DCHECK_NULL(index_);
80+
DCHECK(is_valid());
8181
return creator->AddData(context, GetJSArray());
8282
}
8383

@@ -100,7 +100,7 @@ inline void AliasedBufferBase<NativeT, V8T>::Deserialize(
100100
template <typename NativeT, typename V8T>
101101
AliasedBufferBase<NativeT, V8T>& AliasedBufferBase<NativeT, V8T>::operator=(
102102
AliasedBufferBase<NativeT, V8T>&& that) noexcept {
103-
DCHECK_NULL(index_);
103+
DCHECK(is_valid());
104104
this->~AliasedBufferBase();
105105
isolate_ = that.isolate_;
106106
count_ = that.count_;
@@ -116,7 +116,7 @@ AliasedBufferBase<NativeT, V8T>& AliasedBufferBase<NativeT, V8T>::operator=(
116116

117117
template <typename NativeT, typename V8T>
118118
v8::Local<V8T> AliasedBufferBase<NativeT, V8T>::GetJSArray() const {
119-
DCHECK_NULL(index_);
119+
DCHECK(is_valid());
120120
return js_array_.Get(isolate_);
121121
}
122122

@@ -126,6 +126,20 @@ void AliasedBufferBase<NativeT, V8T>::Release() {
126126
js_array_.Reset();
127127
}
128128

129+
template <typename NativeT, typename V8T>
130+
inline void AliasedBufferBase<NativeT, V8T>::WeakCallback(
131+
const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data) {
132+
AliasedBufferBase<NativeT, V8T>* buffer = data.GetParameter();
133+
DCHECK(buffer->is_valid());
134+
buffer->cleared_ = true;
135+
}
136+
137+
template <typename NativeT, typename V8T>
138+
inline void AliasedBufferBase<NativeT, V8T>::MakeWeak() {
139+
DCHECK(is_valid());
140+
js_array_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
141+
}
142+
129143
template <typename NativeT, typename V8T>
130144
v8::Local<v8::ArrayBuffer> AliasedBufferBase<NativeT, V8T>::GetArrayBuffer()
131145
const {
@@ -134,7 +148,7 @@ v8::Local<v8::ArrayBuffer> AliasedBufferBase<NativeT, V8T>::GetArrayBuffer()
134148

135149
template <typename NativeT, typename V8T>
136150
inline const NativeT* AliasedBufferBase<NativeT, V8T>::GetNativeBuffer() const {
137-
DCHECK_NULL(index_);
151+
DCHECK(is_valid());
138152
return buffer_;
139153
}
140154

@@ -147,22 +161,22 @@ template <typename NativeT, typename V8T>
147161
inline void AliasedBufferBase<NativeT, V8T>::SetValue(const size_t index,
148162
NativeT value) {
149163
DCHECK_LT(index, count_);
150-
DCHECK_NULL(index_);
164+
DCHECK(is_valid());
151165
buffer_[index] = value;
152166
}
153167

154168
template <typename NativeT, typename V8T>
155169
inline const NativeT AliasedBufferBase<NativeT, V8T>::GetValue(
156170
const size_t index) const {
157-
DCHECK_NULL(index_);
171+
DCHECK(is_valid());
158172
DCHECK_LT(index, count_);
159173
return buffer_[index];
160174
}
161175

162176
template <typename NativeT, typename V8T>
163177
typename AliasedBufferBase<NativeT, V8T>::Reference
164178
AliasedBufferBase<NativeT, V8T>::operator[](size_t index) {
165-
DCHECK_NULL(index_);
179+
DCHECK(is_valid());
166180
return Reference(this, index);
167181
}
168182

@@ -178,7 +192,7 @@ size_t AliasedBufferBase<NativeT, V8T>::Length() const {
178192

179193
template <typename NativeT, typename V8T>
180194
void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
181-
DCHECK_NULL(index_);
195+
DCHECK(is_valid());
182196
DCHECK_GE(new_capacity, count_);
183197
DCHECK_EQ(byte_offset_, 0);
184198
const v8::HandleScope handle_scope(isolate_);
@@ -206,6 +220,11 @@ void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
206220
count_ = new_capacity;
207221
}
208222

223+
template <typename NativeT, typename V8T>
224+
inline bool AliasedBufferBase<NativeT, V8T>::is_valid() const {
225+
return index_ == nullptr && !cleared_;
226+
}
227+
209228
template <typename NativeT, typename V8T>
210229
inline size_t AliasedBufferBase<NativeT, V8T>::SelfSize() const {
211230
return sizeof(*this);

‎src/aliased_buffer.h

+12
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,14 @@ class AliasedBufferBase : public MemoryRetainer {
117117

118118
void Release();
119119

120+
/**
121+
* Make the global reference to the typed array weak. The caller must make
122+
* sure that no operation can be done on the AliasedBuffer when the typed
123+
* array becomes unreachable. Usually this means the caller must maintain
124+
* a JS reference to the typed array from JS object.
125+
*/
126+
inline void MakeWeak();
127+
120128
/**
121129
* Get the underlying v8::ArrayBuffer underlying the TypedArray and
122130
* overlaying the native buffer
@@ -164,11 +172,15 @@ class AliasedBufferBase : public MemoryRetainer {
164172
inline void MemoryInfo(node::MemoryTracker* tracker) const override;
165173

166174
private:
175+
inline bool is_valid() const;
176+
static inline void WeakCallback(
177+
const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data);
167178
v8::Isolate* isolate_ = nullptr;
168179
size_t count_ = 0;
169180
size_t byte_offset_ = 0;
170181
NativeT* buffer_ = nullptr;
171182
v8::Global<V8T> js_array_;
183+
bool cleared_ = false;
172184

173185
// Deserialize data
174186
const AliasedBufferIndex* index_ = nullptr;

‎src/encoding_binding.cc

+26-13
Original file line numberDiff line numberDiff line change
@@ -28,30 +28,41 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const {
2828
encode_into_results_buffer_);
2929
}
3030

31-
BindingData::BindingData(Realm* realm, v8::Local<v8::Object> object)
31+
BindingData::BindingData(Realm* realm,
32+
v8::Local<v8::Object> object,
33+
InternalFieldInfo* info)
3234
: SnapshotableObject(realm, object, type_int),
33-
encode_into_results_buffer_(realm->isolate(), kEncodeIntoResultsLength) {
34-
object
35-
->Set(realm->context(),
36-
FIXED_ONE_BYTE_STRING(realm->isolate(), "encodeIntoResults"),
37-
encode_into_results_buffer_.GetJSArray())
38-
.Check();
35+
encode_into_results_buffer_(
36+
realm->isolate(),
37+
kEncodeIntoResultsLength,
38+
MAYBE_FIELD_PTR(info, encode_into_results_buffer)) {
39+
if (info == nullptr) {
40+
object
41+
->Set(realm->context(),
42+
FIXED_ONE_BYTE_STRING(realm->isolate(), "encodeIntoResults"),
43+
encode_into_results_buffer_.GetJSArray())
44+
.Check();
45+
} else {
46+
encode_into_results_buffer_.Deserialize(realm->context());
47+
}
48+
encode_into_results_buffer_.MakeWeak();
3949
}
4050

4151
bool BindingData::PrepareForSerialization(Local<Context> context,
4252
v8::SnapshotCreator* creator) {
43-
// We'll just re-initialize the buffers in the constructor since their
44-
// contents can be thrown away once consumed in the previous call.
45-
encode_into_results_buffer_.Release();
53+
DCHECK_NULL(internal_field_info_);
54+
internal_field_info_ = InternalFieldInfoBase::New<InternalFieldInfo>(type());
55+
internal_field_info_->encode_into_results_buffer =
56+
encode_into_results_buffer_.Serialize(context, creator);
4657
// Return true because we need to maintain the reference to the binding from
4758
// JS land.
4859
return true;
4960
}
5061

5162
InternalFieldInfoBase* BindingData::Serialize(int index) {
5263
DCHECK_EQ(index, BaseObject::kEmbedderType);
53-
InternalFieldInfo* info =
54-
InternalFieldInfoBase::New<InternalFieldInfo>(type());
64+
InternalFieldInfo* info = internal_field_info_;
65+
internal_field_info_ = nullptr;
5566
return info;
5667
}
5768

@@ -63,7 +74,9 @@ void BindingData::Deserialize(Local<Context> context,
6374
v8::HandleScope scope(context->GetIsolate());
6475
Realm* realm = Realm::GetCurrent(context);
6576
// Recreate the buffer in the constructor.
66-
BindingData* binding = realm->AddBindingData<BindingData>(context, holder);
77+
InternalFieldInfo* casted_info = static_cast<InternalFieldInfo*>(info);
78+
BindingData* binding =
79+
realm->AddBindingData<BindingData>(context, holder, casted_info);
6780
CHECK_NOT_NULL(binding);
6881
}
6982

‎src/encoding_binding.h

+7-3
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,13 @@ class ExternalReferenceRegistry;
1414
namespace encoding_binding {
1515
class BindingData : public SnapshotableObject {
1616
public:
17-
BindingData(Realm* realm, v8::Local<v8::Object> obj);
18-
19-
using InternalFieldInfo = InternalFieldInfoBase;
17+
struct InternalFieldInfo : public node::InternalFieldInfoBase {
18+
AliasedBufferIndex encode_into_results_buffer;
19+
};
2020

21+
BindingData(Realm* realm,
22+
v8::Local<v8::Object> obj,
23+
InternalFieldInfo* info = nullptr);
2124
SERIALIZABLE_OBJECT_METHODS()
2225
SET_BINDING_ID(encoding_binding_data)
2326

@@ -39,6 +42,7 @@ class BindingData : public SnapshotableObject {
3942
private:
4043
static constexpr size_t kEncodeIntoResultsLength = 2;
4144
AliasedUint32Array encode_into_results_buffer_;
45+
InternalFieldInfo* internal_field_info_ = nullptr;
4246
};
4347

4448
} // namespace encoding_binding

‎src/node_file.cc

+61-32
Original file line numberDiff line numberDiff line change
@@ -2717,33 +2717,56 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const {
27172717
file_handle_read_wrap_freelist);
27182718
}
27192719

2720-
BindingData::BindingData(Realm* realm, v8::Local<v8::Object> wrap)
2720+
BindingData::BindingData(Realm* realm,
2721+
v8::Local<v8::Object> wrap,
2722+
InternalFieldInfo* info)
27212723
: SnapshotableObject(realm, wrap, type_int),
2722-
stats_field_array(realm->isolate(), kFsStatsBufferLength),
2723-
stats_field_bigint_array(realm->isolate(), kFsStatsBufferLength),
2724-
statfs_field_array(realm->isolate(), kFsStatFsBufferLength),
2725-
statfs_field_bigint_array(realm->isolate(), kFsStatFsBufferLength) {
2724+
stats_field_array(realm->isolate(),
2725+
kFsStatsBufferLength,
2726+
MAYBE_FIELD_PTR(info, stats_field_array)),
2727+
stats_field_bigint_array(realm->isolate(),
2728+
kFsStatsBufferLength,
2729+
MAYBE_FIELD_PTR(info, stats_field_bigint_array)),
2730+
statfs_field_array(realm->isolate(),
2731+
kFsStatFsBufferLength,
2732+
MAYBE_FIELD_PTR(info, statfs_field_array)),
2733+
statfs_field_bigint_array(
2734+
realm->isolate(),
2735+
kFsStatFsBufferLength,
2736+
MAYBE_FIELD_PTR(info, statfs_field_bigint_array)) {
27262737
Isolate* isolate = realm->isolate();
27272738
Local<Context> context = realm->context();
2728-
wrap->Set(context,
2729-
FIXED_ONE_BYTE_STRING(isolate, "statValues"),
2730-
stats_field_array.GetJSArray())
2731-
.Check();
2732-
2733-
wrap->Set(context,
2734-
FIXED_ONE_BYTE_STRING(isolate, "bigintStatValues"),
2735-
stats_field_bigint_array.GetJSArray())
2736-
.Check();
2737-
2738-
wrap->Set(context,
2739-
FIXED_ONE_BYTE_STRING(isolate, "statFsValues"),
2740-
statfs_field_array.GetJSArray())
2741-
.Check();
27422739

2743-
wrap->Set(context,
2744-
FIXED_ONE_BYTE_STRING(isolate, "bigintStatFsValues"),
2745-
statfs_field_bigint_array.GetJSArray())
2746-
.Check();
2740+
if (info == nullptr) {
2741+
wrap->Set(context,
2742+
FIXED_ONE_BYTE_STRING(isolate, "statValues"),
2743+
stats_field_array.GetJSArray())
2744+
.Check();
2745+
2746+
wrap->Set(context,
2747+
FIXED_ONE_BYTE_STRING(isolate, "bigintStatValues"),
2748+
stats_field_bigint_array.GetJSArray())
2749+
.Check();
2750+
2751+
wrap->Set(context,
2752+
FIXED_ONE_BYTE_STRING(isolate, "statFsValues"),
2753+
statfs_field_array.GetJSArray())
2754+
.Check();
2755+
2756+
wrap->Set(context,
2757+
FIXED_ONE_BYTE_STRING(isolate, "bigintStatFsValues"),
2758+
statfs_field_bigint_array.GetJSArray())
2759+
.Check();
2760+
} else {
2761+
stats_field_array.Deserialize(realm->context());
2762+
stats_field_bigint_array.Deserialize(realm->context());
2763+
statfs_field_array.Deserialize(realm->context());
2764+
statfs_field_bigint_array.Deserialize(realm->context());
2765+
}
2766+
stats_field_array.MakeWeak();
2767+
stats_field_bigint_array.MakeWeak();
2768+
statfs_field_array.MakeWeak();
2769+
statfs_field_bigint_array.MakeWeak();
27472770
}
27482771

27492772
void BindingData::Deserialize(Local<Context> context,
@@ -2753,28 +2776,34 @@ void BindingData::Deserialize(Local<Context> context,
27532776
DCHECK_EQ(index, BaseObject::kEmbedderType);
27542777
HandleScope scope(context->GetIsolate());
27552778
Realm* realm = Realm::GetCurrent(context);
2756-
BindingData* binding = realm->AddBindingData<BindingData>(context, holder);
2779+
InternalFieldInfo* casted_info = static_cast<InternalFieldInfo*>(info);
2780+
BindingData* binding =
2781+
realm->AddBindingData<BindingData>(context, holder, casted_info);
27572782
CHECK_NOT_NULL(binding);
27582783
}
27592784

27602785
bool BindingData::PrepareForSerialization(Local<Context> context,
27612786
v8::SnapshotCreator* creator) {
27622787
CHECK(file_handle_read_wrap_freelist.empty());
2763-
// We'll just re-initialize the buffers in the constructor since their
2764-
// contents can be thrown away once consumed in the previous call.
2765-
stats_field_array.Release();
2766-
stats_field_bigint_array.Release();
2767-
statfs_field_array.Release();
2768-
statfs_field_bigint_array.Release();
2788+
DCHECK_NULL(internal_field_info_);
2789+
internal_field_info_ = InternalFieldInfoBase::New<InternalFieldInfo>(type());
2790+
internal_field_info_->stats_field_array =
2791+
stats_field_array.Serialize(context, creator);
2792+
internal_field_info_->stats_field_bigint_array =
2793+
stats_field_bigint_array.Serialize(context, creator);
2794+
internal_field_info_->statfs_field_array =
2795+
statfs_field_array.Serialize(context, creator);
2796+
internal_field_info_->statfs_field_bigint_array =
2797+
statfs_field_bigint_array.Serialize(context, creator);
27692798
// Return true because we need to maintain the reference to the binding from
27702799
// JS land.
27712800
return true;
27722801
}
27732802

27742803
InternalFieldInfoBase* BindingData::Serialize(int index) {
27752804
DCHECK_EQ(index, BaseObject::kEmbedderType);
2776-
InternalFieldInfo* info =
2777-
InternalFieldInfoBase::New<InternalFieldInfo>(type());
2805+
InternalFieldInfo* info = internal_field_info_;
2806+
internal_field_info_ = nullptr;
27782807
return info;
27792808
}
27802809

0 commit comments

Comments
 (0)