Skip to content

Commit 2d99c77

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 2d99c77

13 files changed

+230
-102
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: 28 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,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

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)