Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: use v8::LocalVector instead of MaybeStackBuffer<Value> #54934

Closed
wants to merge 3 commits into from
+66 −54
Closed

src: use v8::LocalVector instead of MaybeStackBuffer<Value> #54934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73f5416
src: use v8::LocalVector instead of MaybeStackBuffer<Value>
jasnell Sep 13, 2024
0ee1ffd
fixup! src: use v8::LocalVector instead of MaybeStackBuffer<Value>
jasnell Sep 13, 2024
a7bee9b
fixup! fixup! src: use v8::LocalVector instead of MaybeStackBuffer<Va…
jasnell Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/api/callback.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ using v8::Function;
using v8::HandleScope;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::Object;
using v8::String;
Expand Down Expand Up @@ -215,14 +216,14 @@ MaybeLocal<Value> InternalMakeCallback(Environment* env,

Local<Context> context = env->context();
if (use_async_hooks_trampoline) {
MaybeStackBuffer<Local<Value>, 16> args(3 + argc);
LocalVector<Value> args(env->isolate(), 3 + argc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you benchmark this? The point of MaybeStackBuffer is to avoid an extra heap allocation, as the name implies, a benefit that gets lost when switching to LocalVector

Copy link
Member

@joyeecheung joyeecheung Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably don't have a choice according to https://chromium-review.googlesource.com/c/v8/v8/+/4905902 - the Local handles are not supposed to be heap-allocated in the usual fashion, so a MaybeStackBuffer<Local<>> that heap-allocates (when the size > kStackStorageSize) is invalid in the first place and we were probably just being lucky to dodge some memory failures that could be caused by it (or maybe we'll get hit when V8 enables conservative stack scanning). LocalVector was added as a V8-acknowledged way to properly heap allocate them.

Copy link
Member

@joyeecheung joyeecheung Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, another way to go with this while preserving the stack-allocation part may be to add a different MaybeStackBuffer that uses LocalVector when it goes heap-allocated, or add some specialization to MaybeStackBuffer so that the heap-allocated part uses LocalVector when the element type is Local (seems messier though).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung Yeah, those sound like good ideas for solving this problem to me 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: How can we disallow heap allocation on Local handles? What's the best way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targos@0982497

It breaks the world: https://ci.nodejs.org/view/All/job/node-test-commit-arm-debug/15086/nodes=ubuntu2204_debug-arm64/console

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if it breaks here as well. Thanks for the tip @targos: cloudflare/workerd#2752

args[0] = v8::Number::New(env->isolate(), asyncContext.async_id);
args[1] = resource;
args[2] = callback;
for (int i = 0; i < argc; i++) {
args[i + 3] = argv[i];
}
ret = hook_cb->Call(context, recv, args.length(), &args[0]);
ret = hook_cb->Call(context, recv, args.size(), args.data());
} else {
ret = callback->Call(context, recv, argc, argv);
}
Expand Down
5 changes: 3 additions & 2 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ using v8::Isolate;
using v8::Just;
using v8::JustVoid;
using v8::Local;
using v8::LocalVector;
using v8::Maybe;
using v8::Nothing;
using v8::Null;
Expand Down Expand Up @@ -193,11 +194,11 @@ Local<Array> AddrTTLToArray(
Environment* env,
const T* addrttls,
size_t naddrttls) {
MaybeStackBuffer<Local<Value>, 8> ttls(naddrttls);
LocalVector<Value> ttls(env->isolate(), naddrttls);
for (size_t i = 0; i < naddrttls; i++)
ttls[i] = Integer::NewFromUnsigned(env->isolate(), addrttls[i].ttl);

return Array::New(env->isolate(), ttls.out(), naddrttls);
return Array::New(env->isolate(), ttls.data(), ttls.size());
}

int ParseGeneralReply(
Expand Down
7 changes: 4 additions & 3 deletions src/crypto/crypto_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ using v8::Context;
using v8::EscapableHandleScope;
using v8::Integer;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::Object;
using v8::String;
Expand Down Expand Up @@ -387,7 +388,7 @@ MaybeLocal<Array> GetClientHelloCiphers(
const unsigned char* buf;
size_t len = SSL_client_hello_get0_ciphers(ssl.get(), &buf);
size_t count = len / 2;
MaybeStackBuffer<Local<Value>, 16> ciphers(count);
LocalVector<Value> ciphers(env->isolate(), count);
int j = 0;
for (size_t n = 0; n < len; n += 2) {
const SSL_CIPHER* cipher = SSL_CIPHER_find(ssl.get(), buf);
Expand All @@ -409,8 +410,8 @@ MaybeLocal<Array> GetClientHelloCiphers(
}
ciphers[j++] = obj;
}
Local<Array> ret = Array::New(env->isolate(), ciphers.out(), count);
return scope.Escape(ret);
return scope.Escape(
Array::New(env->isolate(), ciphers.data(), ciphers.size()));
}


Expand Down
5 changes: 3 additions & 2 deletions src/crypto/crypto_tls.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ using v8::HandleScope;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::Null;
using v8::Object;
Expand Down Expand Up @@ -1869,7 +1870,7 @@ void TLSWrap::GetSharedSigalgs(const FunctionCallbackInfo<Value>& args) {
SSL* ssl = w->ssl_.get();
int nsig = SSL_get_shared_sigalgs(ssl, 0, nullptr, nullptr, nullptr, nullptr,
nullptr);
MaybeStackBuffer<Local<Value>, 16> ret_arr(nsig);
LocalVector<Value> ret_arr(env->isolate(), nsig);

for (int i = 0; i < nsig; i++) {
int hash_nid;
Expand Down Expand Up @@ -1937,7 +1938,7 @@ void TLSWrap::GetSharedSigalgs(const FunctionCallbackInfo<Value>& args) {
}

args.GetReturnValue().Set(
Array::New(env->isolate(), ret_arr.out(), ret_arr.length()));
Array::New(env->isolate(), ret_arr.data(), ret_arr.size()));
}

void TLSWrap::ExportKeyingMaterial(const FunctionCallbackInfo<Value>& args) {
Expand Down
5 changes: 3 additions & 2 deletions src/crypto/crypto_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,8 @@ void ThrowCryptoError(Environment* env,

class CipherPushContext {
public:
inline explicit CipherPushContext(Environment* env) : env_(env) {}
inline explicit CipherPushContext(Environment* env)
: list_(env->isolate()), env_(env) {}

inline void push_back(const char* str) {
list_.emplace_back(OneByteString(env_->isolate(), str));
Expand All @@ -558,7 +559,7 @@ class CipherPushContext {
}

private:
std::vector<v8::Local<v8::Value>> list_;
v8::LocalVector<v8::Value> list_;
Environment* env_;
};

Expand Down
6 changes: 4 additions & 2 deletions src/crypto/crypto_x509.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ using v8::FunctionTemplate;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Object;
Expand Down Expand Up @@ -265,7 +266,7 @@ MaybeLocal<Value> GetKeyUsage(Environment* env, const ncrypto::X509View& cert) {
X509_get_ext_d2i(cert.get(), NID_ext_key_usage, nullptr, nullptr)));
if (eku) {
const int count = sk_ASN1_OBJECT_num(eku.get());
MaybeStackBuffer<Local<Value>, 16> ext_key_usage(count);
LocalVector<Value> ext_key_usage(env->isolate(), count);
char buf[256];

int j = 0;
Expand All @@ -276,7 +277,8 @@ MaybeLocal<Value> GetKeyUsage(Environment* env, const ncrypto::X509View& cert) {
}
}

return Array::New(env->isolate(), ext_key_usage.out(), count);
return Array::New(
env->isolate(), ext_key_usage.data(), ext_key_usage.size());
}

return Undefined(env->isolate());
Expand Down
8 changes: 4 additions & 4 deletions src/js_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ using v8::HandleScope;
using v8::Int32;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::Object;
using v8::TryCatch;
using v8::Value;
Expand Down Expand Up @@ -117,16 +118,15 @@ int JSStream::DoWrite(WriteWrap* w,
HandleScope scope(env()->isolate());
Context::Scope context_scope(env()->context());

MaybeStackBuffer<Local<Value>, 16> bufs_arr(count);
LocalVector<Value> bufs_arr(env()->isolate(), count);
for (size_t i = 0; i < count; i++) {
bufs_arr[i] =
Buffer::Copy(env(), bufs[i].base, bufs[i].len).ToLocalChecked();
}

Local<Value> argv[] = {
w->object(),
Array::New(env()->isolate(), bufs_arr.out(), count)
};
w->object(),
Array::New(env()->isolate(), bufs_arr.data(), bufs_arr.size())};

TryCatchScope try_catch(env());
Local<Value> value;
Expand Down
9 changes: 5 additions & 4 deletions src/js_udp_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ using v8::HandleScope;
using v8::Int32;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::Object;
using v8::Value;

Expand Down Expand Up @@ -97,7 +98,7 @@ ssize_t JSUDPWrap::Send(uv_buf_t* bufs,
int64_t value_int = JS_EXCEPTION_PENDING;
size_t total_len = 0;

MaybeStackBuffer<Local<Value>, 16> buffers(nbufs);
LocalVector<Value> buffers(env()->isolate(), nbufs);
for (size_t i = 0; i < nbufs; i++) {
buffers[i] = Buffer::Copy(env(), bufs[i].base, bufs[i].len)
.ToLocalChecked();
Expand All @@ -108,9 +109,9 @@ ssize_t JSUDPWrap::Send(uv_buf_t* bufs,
if (!AddressToJS(env(), addr).ToLocal(&address)) return value_int;

Local<Value> args[] = {
listener()->CreateSendWrap(total_len)->object(),
Array::New(env()->isolate(), buffers.out(), nbufs),
address,
listener()->CreateSendWrap(total_len)->object(),
Array::New(env()->isolate(), buffers.data(), buffers.size()),
address,
};

if (!MakeCallback(env()->onwrite_string(), arraysize(args), args)
Expand Down
5 changes: 3 additions & 2 deletions src/node_dir.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ using v8::HandleScope;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::Null;
using v8::Number;
Expand Down Expand Up @@ -212,7 +213,7 @@ static MaybeLocal<Array> DirentListToArray(
int num,
enum encoding encoding,
Local<Value>* err_out) {
MaybeStackBuffer<Local<Value>, 64> entries(num * 2);
LocalVector<Value> entries(env->isolate(), num * 2);

// Return an array of all read filenames.
int j = 0;
Expand All @@ -233,7 +234,7 @@ static MaybeLocal<Array> DirentListToArray(
entries[j++] = Integer::New(env->isolate(), ents[i].type);
}

return Array::New(env->isolate(), entries.out(), j);
return Array::New(env->isolate(), entries.data(), entries.size());
}

static void AfterDirRead(uv_fs_t* req) {
Expand Down
8 changes: 5 additions & 3 deletions src/node_env_var.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ using v8::Isolate;
using v8::Just;
using v8::JustVoid;
using v8::Local;
using v8::LocalVector;
using v8::Maybe;
using v8::MaybeLocal;
using v8::Name;
Expand Down Expand Up @@ -196,8 +197,8 @@ Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
auto cleanup = OnScopeLeave([&]() { uv_os_free_environ(items, count); });
CHECK_EQ(uv_os_environ(&items, &count), 0);

MaybeStackBuffer<Local<Value>, 256> env_v(count);
int env_v_index = 0;
LocalVector<Value> env_v(isolate, count);
size_t env_v_index = 0;
for (int i = 0; i < count; i++) {
#ifdef _WIN32
// If the key starts with '=' it is a hidden environment variable.
Expand All @@ -211,7 +212,8 @@ Local<Array> RealEnvStore::Enumerate(Isolate* isolate) const {
env_v[env_v_index++] = str.ToLocalChecked();
}

return Array::New(isolate, env_v.out(), env_v_index);
CHECK_LE(env_v_index, env_v.size());
return Array::New(isolate, env_v.data(), env_v_index);
}

std::shared_ptr<KVStore> KVStore::Clone(Isolate* isolate) const {
Expand Down
17 changes: 9 additions & 8 deletions src/node_http2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ using v8::HandleScope;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::NewStringType;
using v8::Number;
Expand Down Expand Up @@ -1447,8 +1448,8 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
// this way for performance reasons (it's faster to generate and pass an
// array than it is to generate and pass the object).

MaybeStackBuffer<Local<Value>, 64> headers_v(stream->headers_count() * 2);
MaybeStackBuffer<Local<Value>, 32> sensitive_v(stream->headers_count());
LocalVector<Value> headers_v(isolate, stream->headers_count() * 2);
LocalVector<Value> sensitive_v(isolate, stream->headers_count());
size_t sensitive_count = 0;

stream->TransferHeaders([&](const Http2Header& header, size_t i) {
Expand All @@ -1463,12 +1464,12 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
stream->current_headers_length_ = 0;

Local<Value> args[] = {
stream->object(),
Integer::New(isolate, id),
Integer::New(isolate, stream->headers_category()),
Integer::New(isolate, frame->hd.flags),
Array::New(isolate, headers_v.out(), headers_v.length()),
Array::New(isolate, sensitive_v.out(), sensitive_count),
stream->object(),
Integer::New(isolate, id),
Integer::New(isolate, stream->headers_category()),
Integer::New(isolate, frame->hd.flags),
Array::New(isolate, headers_v.data(), headers_v.size()),
Array::New(isolate, sensitive_v.data(), sensitive_count),
};
MakeCallback(env()->http2session_on_headers_function(),
arraysize(args), args);
Expand Down
1 change: 1 addition & 0 deletions src/node_messaging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace worker {
class MessagePortData;
class MessagePort;

// TODO(@jasnell): Can this be implemented in terms of v8::LocalVector instead
typedef MaybeStackBuffer<v8::Local<v8::Value>, 8> TransferList;

// Used to represent the in-flight structure of an object that is being
Expand Down
8 changes: 4 additions & 4 deletions src/node_sockaddr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ using v8::FunctionTemplate;
using v8::Int32;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::MaybeLocal;
using v8::Object;
using v8::Uint32;
Expand Down Expand Up @@ -503,15 +504,14 @@ std::string SocketAddressBlockList::SocketAddressMaskRule::ToString() {

MaybeLocal<Array> SocketAddressBlockList::ListRules(Environment* env) {
Mutex::ScopedLock lock(mutex_);
std::vector<Local<Value>> rules;
v8::LocalVector<Value> rules(env->isolate());
if (!ListRules(env, &rules))
return MaybeLocal<Array>();
return Array::New(env->isolate(), rules.data(), rules.size());
}

bool SocketAddressBlockList::ListRules(
Environment* env,
std::vector<v8::Local<v8::Value>>* rules) {
bool SocketAddressBlockList::ListRules(Environment* env,
LocalVector<Value>* rules) {
if (parent_ && !parent_->ListRules(env, rules))
return false;
for (const auto& rule : rules_) {
Expand Down
4 changes: 1 addition & 3 deletions src/node_sockaddr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,7 @@ class SocketAddressBlockList : public MemoryRetainer {
SET_SELF_SIZE(SocketAddressBlockList)

private:
bool ListRules(
Environment* env,
std::vector<v8::Local<v8::Value>>* vec);
bool ListRules(Environment* env, v8::LocalVector<v8::Value>* vec);

std::shared_ptr<SocketAddressBlockList> parent_;
std::list<std::unique_ptr<Rule>> rules_;
Expand Down
10 changes: 5 additions & 5 deletions src/node_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ using v8::HeapStatistics;
using v8::Integer;
using v8::Isolate;
using v8::Local;
using v8::LocalVector;
using v8::Object;
using v8::ScriptCompiler;
using v8::String;
Expand Down Expand Up @@ -446,17 +447,16 @@ void Initialize(Local<Object> target,
// Heap space names are extracted once and exposed to JavaScript to
// avoid excessive creation of heap space name Strings.
HeapSpaceStatistics s;
MaybeStackBuffer<Local<Value>, 16> heap_spaces(number_of_heap_spaces);
LocalVector<Value> heap_spaces(env->isolate(), number_of_heap_spaces);
for (size_t i = 0; i < number_of_heap_spaces; i++) {
env->isolate()->GetHeapSpaceStatistics(&s, i);
heap_spaces[i] = String::NewFromUtf8(env->isolate(), s.space_name())
.ToLocalChecked();
}
target
->Set(
context,
FIXED_ONE_BYTE_STRING(env->isolate(), "kHeapSpaces"),
Array::New(env->isolate(), heap_spaces.out(), number_of_heap_spaces))
->Set(context,
FIXED_ONE_BYTE_STRING(env->isolate(), "kHeapSpaces"),
Array::New(env->isolate(), heap_spaces.data(), heap_spaces.size()))
.Check();

SetMethod(context,
Expand Down
Loading
Loading