Skip to content

Commit b68cedd

Browse files
joyeecheungjoyeecheung
authored
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. PR-URL: #47354 Refs: #47353 Reviewed-By: Chengzhong Wu <[email protected]>
1 parent df15a47 commit b68cedd

13 files changed

+230
-101
lines changed

‎lib/fs.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,6 @@ const {
5959
const pathModule = require('path');
6060
const { isArrayBufferView } = require('internal/util/types');
6161

62-
// We need to get the statValues from the binding at the callsite since
63-
// it's re-initialized after deserialization.
64-
6562
const binding = internalBinding('fs');
6663

6764
const { createBlobFromFilePath } = require('internal/blob');
@@ -78,7 +75,10 @@ const {
7875
uvException,
7976
} = require('internal/errors');
8077

81-
const { FSReqCallback } = binding;
78+
const {
79+
FSReqCallback,
80+
statValues,
81+
} = binding;
8282
const { toPathIfFileURL } = require('internal/url');
8383
const {
8484
customPromisifyArgs: kCustomPromisifyArgsSymbol,
@@ -2569,8 +2569,8 @@ function realpathSync(p, options) {
25692569

25702570
// Continue if not a symlink, break if a pipe/socket
25712571
if (knownHard.has(base) || cache?.get(base) === base) {
2572-
if (isFileType(binding.statValues, S_IFIFO) ||
2573-
isFileType(binding.statValues, S_IFSOCK)) {
2572+
if (isFileType(statValues, S_IFIFO) ||
2573+
isFileType(statValues, S_IFSOCK)) {
25742574
break;
25752575
}
25762576
continue;
@@ -2727,8 +2727,8 @@ function realpath(p, options, callback) {
27272727

27282728
// Continue if not a symlink, break if a pipe/socket
27292729
if (knownHard.has(base)) {
2730-
if (isFileType(binding.statValues, S_IFIFO) ||
2731-
isFileType(binding.statValues, S_IFSOCK)) {
2730+
if (isFileType(statValues, S_IFIFO) ||
2731+
isFileType(statValues, S_IFSOCK)) {
27322732
return callback(null, encodeRealpathResult(p, options));
27332733
}
27342734
return process.nextTick(LOOP);

‎lib/internal/encoding.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ const {
5151
const binding = internalBinding('encoding_binding');
5252
const {
5353
encodeInto,
54+
encodeIntoResults,
5455
encodeUtf8String,
5556
decodeUTF8,
5657
} = binding;
@@ -341,7 +342,7 @@ class TextEncoder {
341342
encodeInto(src, dest);
342343
// We need to read from the binding here since the buffer gets refreshed
343344
// from the snapshot.
344-
const { 0: read, 1: written } = binding.encodeIntoResults;
345+
const { 0: read, 1: written } = encodeIntoResults;
345346
return { read, written };
346347
}
347348

‎lib/v8.js

Lines changed: 7 additions & 3 deletions
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

Lines changed: 29 additions & 9 deletions
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,21 @@ 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+
buffer->js_array_.Reset();
136+
}
137+
138+
template <typename NativeT, typename V8T>
139+
inline void AliasedBufferBase<NativeT, V8T>::MakeWeak() {
140+
DCHECK(is_valid());
141+
js_array_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
142+
}
143+
129144
template <typename NativeT, typename V8T>
130145
v8::Local<v8::ArrayBuffer> AliasedBufferBase<NativeT, V8T>::GetArrayBuffer()
131146
const {
@@ -134,7 +149,7 @@ v8::Local<v8::ArrayBuffer> AliasedBufferBase<NativeT, V8T>::GetArrayBuffer()
134149

135150
template <typename NativeT, typename V8T>
136151
inline const NativeT* AliasedBufferBase<NativeT, V8T>::GetNativeBuffer() const {
137-
DCHECK_NULL(index_);
152+
DCHECK(is_valid());
138153
return buffer_;
139154
}
140155

@@ -147,22 +162,22 @@ template <typename NativeT, typename V8T>
147162
inline void AliasedBufferBase<NativeT, V8T>::SetValue(const size_t index,
148163
NativeT value) {
149164
DCHECK_LT(index, count_);
150-
DCHECK_NULL(index_);
165+
DCHECK(is_valid());
151166
buffer_[index] = value;
152167
}
153168

154169
template <typename NativeT, typename V8T>
155170
inline const NativeT AliasedBufferBase<NativeT, V8T>::GetValue(
156171
const size_t index) const {
157-
DCHECK_NULL(index_);
172+
DCHECK(is_valid());
158173
DCHECK_LT(index, count_);
159174
return buffer_[index];
160175
}
161176

162177
template <typename NativeT, typename V8T>
163178
typename AliasedBufferBase<NativeT, V8T>::Reference
164179
AliasedBufferBase<NativeT, V8T>::operator[](size_t index) {
165-
DCHECK_NULL(index_);
180+
DCHECK(is_valid());
166181
return Reference(this, index);
167182
}
168183

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

179194
template <typename NativeT, typename V8T>
180195
void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
181-
DCHECK_NULL(index_);
196+
DCHECK(is_valid());
182197
DCHECK_GE(new_capacity, count_);
183198
DCHECK_EQ(byte_offset_, 0);
184199
const v8::HandleScope handle_scope(isolate_);
@@ -206,6 +221,11 @@ void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity) {
206221
count_ = new_capacity;
207222
}
208223

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

‎src/aliased_buffer.h

Lines changed: 12 additions & 0 deletions
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

Lines changed: 26 additions & 13 deletions
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

Lines changed: 7 additions & 3 deletions
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

0 commit comments

Comments
 (0)