Skip to content

ffi: allocate string argument storage lazily - #66178

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
soulee-dev:ffi-lazy-string-storage
Sep 25, 2026
Merged

nodejs-github-bot merged 1 commit into
nodejs:mainfrom
soulee-dev:ffi-lazy-string-storage

Conversation

@soulee-dev

Copy link
Copy Markdown
Contributor

InvokeFunction reserves a temporary std::vector<std::string> on every call with arguments, even when none of the arguments are strings. Defer that reservation until the first string argument needs to be copied, avoiding an allocation and deallocation for calls without string arguments.

The first string still reserves capacity for the full argument count before its pointer is saved. Subsequent strings therefore cannot reallocate the vector and invalidate pointers already prepared for the native call.

Add a test that exercises native argument conversion with multiple strings, empty and long strings, Unicode, mixed string/Buffer arguments, and errors while converting a later argument.

Local macOS ARM64 comparison using benchmark/compare.js, with 10 samples per binary and the default iteration counts across all 40 FFI benchmark configurations:

                                                              confidence   improvement   accuracy (*)    (**)   (***)
ffi/many-args.js n=10000000                                   ***             +5.97 %   ±2.33%  ±3.26%  ±4.60%
ffi/sum-5-i32.js n=10000000                                   ***            +11.18 %   ±2.49%  ±3.42%  ±4.67%

Both improvements remained significant at the 5% level after the runner's Holm-Bonferroni correction. The other 38 configurations showed no statistically significant difference in this run.

Validation:

  • Release build with Ninja (--without-node-snapshot --without-node-code-cache).
  • All 16 FFI test files passed, including the new test.
  • C++ lint, import checks, and ESLint for the new test passed.

The full Node.js test suite and cross-platform CI have not been run.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 21, 2026
Reserve temporary string storage in InvokeFunction only when the
first string argument is encountered. Calls without string arguments
avoid allocating and freeing an unused vector buffer.

Reserve capacity for all arguments before saving the first string
pointer so subsequent strings cannot invalidate it. Add coverage for
multiple strings, mixed Buffer arguments, and errors during argument
conversion.

Signed-off-by: Soul Lee <[email protected]>
@soulee-dev
soulee-dev force-pushed the ffi-lazy-string-storage branch from c85110b to aa0c8b5 Compare September 21, 2026 11:57
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.28%. Comparing base (97af3d7) to head (aa0c8b5).
⚠️ Report is 123 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66178      +/-   ##
==========================================
- Coverage   90.29%   90.28%   -0.01%     
==========================================
  Files         790      790              
  Lines      272021   272045      +24     
  Branches    51917    51936      +19     
==========================================
+ Hits       245611   245612       +1     
- Misses      16903    16941      +38     
+ Partials     9507     9492      -15     
Files with missing lines Coverage Δ
src/node_ffi.cc 72.40% <100.00%> (+0.41%) ⬆️

... and 53 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.

@daeyeon daeyeon added the ffi Issues and PRs related to experimental Foreign Function Interface support. label Sep 22, 2026
@daeyeon daeyeon added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Sep 24, 2026
@github-actions github-actions Bot removed the resume-ci Add this label to resume the latest eligible Jenkins CI run on a PR with an approving review. label Sep 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 25, 2026
@nodejs-github-bot nodejs-github-bot added the lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. label Sep 25, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 6c59f35 into nodejs:main Sep 25, 2026
88 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 6c59f35

@nodejs-github-bot nodejs-github-bot removed commit-queue PRs queued for automated landing through the Commit Queue. lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. labels Sep 25, 2026
aduh95 pushed a commit that referenced this pull request Sep 27, 2026
Reserve temporary string storage in InvokeFunction only when the
first string argument is encountered. Calls without string arguments
avoid allocating and freeing an unused vector buffer.

Reserve capacity for all arguments before saving the first string
pointer so subsequent strings cannot invalidate it. Add coverage for
multiple strings, mixed Buffer arguments, and errors during argument
conversion.

Signed-off-by: Soul Lee <[email protected]>
PR-URL: #66178
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants