8000 sqlite: avoid extra copy for large text binds by thisalihassan · Pull Request #61580 · nodejs/node · GitHub
[go: up one dir, main page]
Skip to content

sqlite: avoid extra copy for large text binds#61580

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
thisalihassan:sqlite-bind-text64-avoid-copy
Feb 7, 2026
Merged

sqlite: avoid extra copy for large text binds#61580
nodejs-github-bot merged 1 commit intonodejs:mainfrom
thisalihassan:sqlite-bind-text64-avoid-copy

Conversation

@thisalihassan
Copy link
Contributor
@thisalihassan thisalihassan commented Jan 29, 2026 •
edited
Loading

Summary

  • When binding UTF-8 strings to SQLite statements, transfer ownership of the malloc-backed Utf8Value buffer to SQLite to avoid an extra copy for larger strings.
  • Use sqlite3_bind_blob64() for BLOB parameters to avoid truncation for larger payloads.

@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Jan 29, 2026

Review requested:

  • @nodejs/sqlite

@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. sqlite Issues and PRs related to the SQLite subsystem. labels Jan 29, 2026
@thisalihassan thisalihassan force-pushed the sqlite-bind-text64-avoid-copy branch from ea75fcc to b7abb6f Compare January 29, 2026 21:00
@thisalihassan
Copy link
Contributor Author
thisalihassan commented Jan 29, 2026

Benchmarks (local, macOS, Release build)

Compared with base branch (main)

Summary:

  • INSERT INTO large_text (text_8kb_column) VALUES (?): *** +5.30% (±1.42%)
  • Other insert cases: within ~±1% (not significant), except all_column_types -0.77% (*) (±0.77%)

change primarily improves large text binds; other workloads appear neutral within measurement noise.

@codecov
Copy link
codecov bot commented Jan 29, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (37e4004) to head (da58cfd).
⚠️ Report is 81 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #61580      +/-   ##
==========================================
- Coverage   89.78%   89.77%   -0.01%     
==========================================
  Files         673      673              
  Lines      203775   203852      +77     
  Branches    39166    39181      +15     
==========================================
+ Hits       182956   183017      +61     
- Misses      13139    13153      +14     
- Partials     7680     7682       +2
Files with missing lines Coverage Δ
src/node_sqlite.cc 80.75% <100.00%> (+0.09%) ⬆️

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

@thisalihassan thisalihassan force-pushed the sqlite-bind-text64-avoid-copy branch 2 times, most recently from 92bb01c to 8e4016b Compare January 30, 2026 09:23
@thisalihassan
sqlite: avoid extra copy for large text binds
da58cfd 
When binding UTF-8 strings to prepared statements, transfer ownership of
malloc-backed Utf8Value buffers to SQLite to avoid an extra copy for
large strings. Use sqlite3_bind_blob64() when binding BLOB parameters.
@thisalihassan thisalihassan force-pushed the sqlite-bind-text64-avoid-copy branch from 8e4016b to da58cfd Compare January 30, 2026 09:30
@thisalihassan
Copy link
Contributor Author
thisalihassan commented Jan 30, 2026 •
edited
Loading

Hi @Renegade334 and @mcollina When you have a moment, could you please take a look at this PR and share your feedback? Thanks so much!

mcollina
mcollina approved these changes Jan 31, 2026
View reviewed changes
Copy link
Member
@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2026
@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Jan 31, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/71137/

@thisalihassan
Copy link
Contributor Author
thisalihassan commented Feb 5, 2026

HI @mcollina some flaky tests failed, is it ready to merge?

@geeksilva97 geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 5, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2026
@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Feb 5, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/71204/

@thisalihassan
Copy link
Contributor Author
thisalihassan commented Feb 6, 2026

All CI passed @geeksilva97 should it be merged?

@geeksilva97
Copy link
Contributor
geeksilva97 commented Feb 7, 2026

All CI passed @geeksilva97 should it be merged?

yep. let's do this

@geeksilva97 geeksilva97 added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 7, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 7, 2026
@nodejs-github-bot nodejs-github-bot merged commit 346ad95 into nodejs:main Feb 7, 2026
74 checks passed
@nodejs-github-bot
Copy link
Collaborator
nodejs-github-bot commented Feb 7, 2026

Landed in 346ad95

aduh95 pushed a commit that referenced this pull request Feb 8, 2026
@thisalihassan @aduh95
sqlite: avoid extra copy for large text binds
d4c9b5c 
When binding UTF-8 strings to prepared statements, transfer ownership of
malloc-backed Utf8Value buffers to SQLite to avoid an extra copy for
large strings. Use sqlite3_bind_blob64() when binding BLOB parameters.

PR-URL: #61580
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
@github-actions github-actions bot mentioned this pull request Feb 8, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@Renegade334 Renegade334 Renegade334 approved these changes

@himself65 himself65 himself65 approved these changes

@geeksilva97 geeksilva97 geeksilva97 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@thisalihassan @nodejs-github-bot @geeksilva97 @mcollina @Renegade334 @himself65
0