Skip to content

src: add v8::Local specialization for MaybeStackBuffer - #65159

Open
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/maybestackbuffer-v8-local
Open

src: add v8::Local specialization for MaybeStackBuffer#65159
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/maybestackbuffer-v8-local

Conversation

@jasnell

@jasnell jasnell commented Aug 9, 2026

Copy link
Copy Markdown
Member

Long-term itch. Per v8 rules, we're not supposed to be heap allocating v8::Local's; instead we're supposed to be using v8::LocalVector. Create a specialization of MaybeStackBuffer that uses either a stack array of v8::Locals or v8::LocalVector with some additional utility improvements.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 9, 2026
@jasnell
jasnell requested a review from addaleax August 9, 2026 04:36
@jasnell
jasnell force-pushed the jasnell/maybestackbuffer-v8-local branch 4 times, most recently from 2b1fac0 to f819d7c Compare August 9, 2026 05:29
@nodejs-github-bot

This comment was marked as outdated.

@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.55814% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (0700e74) to head (abf8c83).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/util-inl.h 72.72% 3 Missing and 3 partials ⚠️
src/js_udp_wrap.cc 0.00% 4 Missing ⚠️
src/util.h 89.28% 0 Missing and 3 partials ⚠️
src/node_dir.cc 66.66% 0 Missing and 1 partial ⚠️
src/node_env_var.cc 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65159   +/-   ##
=======================================
  Coverage   90.31%   90.32%           
=======================================
  Files         759      759           
  Lines      248342   248372   +30     
  Branches    46872    46868    -4     
=======================================
+ Hits       224295   224337   +42     
+ Misses      15474    15468    -6     
+ Partials     8573     8567    -6     
Files with missing lines Coverage Δ
src/api/callback.cc 83.17% <100.00%> (ø)
src/cares_wrap.cc 63.07% <100.00%> (ø)
src/crypto/crypto_tls.cc 78.78% <100.00%> (-0.02%) ⬇️
src/js_stream.cc 74.43% <100.00%> (-0.38%) ⬇️
src/node_http2.cc 81.71% <100.00%> (+0.05%) ⬆️
src/node_messaging.cc 82.95% <100.00%> (ø)
src/node_messaging.h 63.15% <ø> (ø)
src/node_v8.cc 87.86% <100.00%> (ø)
src/spawn_sync.cc 68.88% <100.00%> (-0.05%) ⬇️
src/node_dir.cc 70.78% <66.66%> (-0.30%) ⬇️
... and 4 more

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasnell

This comment was marked as resolved.

Long-term itch. Per v8 rules, we're not supposed to be
heap allocating v8::Local's; instead we're supposed to
be using v8::LocalVector. Create a specialization of
MaybeStackBuffer that uses either a stack array of
v8::Locals or v8::LocalVector with some additional
utility improvements.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
jasnell force-pushed the jasnell/maybestackbuffer-v8-local branch from f819d7c to abf8c83 Compare August 9, 2026 15:33
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants