src: use v8::LocalVector instead of MaybeStackBuffer<Value> #54934
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -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); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@joyeecheung Yeah, those sound like good ideas for solving this problem to me 🙂
There was a problem hiding this comment.
Q: How can we disallow heap allocation on Local handles? What's the best way?
There was a problem hiding this comment.
There was a problem hiding this comment.
Let's see if it breaks here as well. Thanks for the tip @targos: cloudflare/workerd#2752
jasnell
force-pushed
the
use-v8-localvector-more
branch
from
7da2d30
to
a7bee9b
Compare
|
Closing given the feedback |
No description provided.