Skip to content

http: batch body spans per parser execute - #67187

Open
araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:http-batch-chunked-body
Open

araujogui wants to merge 2 commits into
nodejs:mainfrom
araujogui:http-batch-chunked-body

Conversation

@araujogui

Copy link
Copy Markdown
Member

Buffer body spans in on_body() and deliver them to JS once per execute() call instead of once per chunk. A body sent as many small chunks no longer costs one C++ to JS call and Buffer per chunk.

Chunks that arrive in the same read now produce a single 'data' event on IncomingMessage. Chunk boundaries were never documented, but code that treats each 'data' event as a full message could break, so this is marked semver-major and needs a CITGM run.

Buffer body spans in on_body() and deliver them to JS once per
execute() call instead of once per chunk. A body sent as many small
chunks no longer costs one C++ to JS call and Buffer per chunk.

Signed-off-by: Guilherme Araújo <[email protected]>
Copilot AI balanced review requested due to automatic review settings November 1, 2026 17:55
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
  • @nodejs/performance

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP parser implementation or http_parser binding. needs-ci PRs that need a full CI run. labels Nov 1, 2026
@araujogui araujogui added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 1, 2026
@ronag

ronag commented Nov 1, 2026

Copy link
Copy Markdown
Member

Any benchmark/perf numbers?

@araujogui

araujogui commented Nov 1, 2026 •

Copy link
Copy Markdown
Member Author

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1903/

Benchmark
++ ./node-master benchmark/compare.js --set CPUSET=0-11 --analyze --old ./node-master --new ./node-pr --filter bench-parser-chunked -- http
                                                             confidence   improvement   accuracy (*)    (**)   (***)
http/bench-parser-chunked.js n=500 len=65536 chunkSize=1     ***          +3726.84 %   ±15.61%  ±21.03%  ±27.92%
http/bench-parser-chunked.js n=500 len=65536 chunkSize=16    ***          +2122.98 %   ±38.26%  ±51.57%  ±68.46%
http/bench-parser-chunked.js n=500 len=65536 chunkSize=256   ***           +522.99 %   ±40.48%  ±54.55%  ±72.41%
http/bench-parser-chunked.js n=500 len=65536 chunkSize=4096  ***            +43.47 %   ±11.18%  ±14.93%  ±19.55%

                                                                -3742.4%           0%           +3742.4%
http/bench-parser-chunked.js n=500 len=65536 chunkSize=1                         |███████████████████  +3726.84% ***
http/bench-parser-chunked.js n=500 len=65536 chunkSize=16                        |██████████░          +2122.98% ***
http/bench-parser-chunked.js n=500 len=65536 chunkSize=256                       |██                   +522.99% ***
http/bench-parser-chunked.js n=500 len=65536 chunkSize=4096                      |                     +43.47% ***

Rates were scaled by 1000x into HdrHistogram (3 significant figures).
Use --scale to adjust precision if needed.

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

The stars above are per-benchmark and uncorrected. Adjusting for the size of
this comparison set (Holm-Bonferroni), 4 comparisons remain significant at 5%.

@ronag
ronag requested a review from mcollina November 1, 2026 19:15

@ronag ronag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

RSLGTM

Comment thread src/node_http_parser.cc
Local<Value> buffer = Buffer::Copy(env, data, length).ToLocalChecked();
// ponytail: clear() keeps capacity (up to one read, 64 KiB) per parser;
// shrink_to_fit() here if pooled parsers' memory becomes a concern.
body_buf_.clear();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can grow capacity-wise unbounded, e.g. one big message can keep the buffer needlessly big, forever

Comment thread src/node_http_parser.cc
body_buf_.assign(pending_body_, pending_body_ + pending_body_len_);
body_buf_.insert(body_buf_.end(), at, at + length);
}
pending_body_len_ += length;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should we maybe have a body max size, and flush if exceeding it? e.g. 64k?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

then the body_buf_ can be reserved to 64k on construction

@codecov

codecov Bot commented Nov 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.71%. Comparing base (97aed16) to head (bfabf27).
⚠️ Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
src/node_http_parser.cc 86.20% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67187      +/-   ##
==========================================
+ Coverage   90.67%   90.71%   +0.03%     
==========================================
  Files         797      797              
  Lines      278773   278808      +35     
  Branches    53607    53615       +8     
==========================================
+ Hits       252778   252916     +138     
+ Misses      16356    16249     -107     
- Partials     9639     9643       +4     
Files with missing lines Coverage Δ
src/node_http_parser.cc 85.22% <86.20%> (-0.11%) ⬇️

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

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++. http_parser Issues and PRs related to the HTTP parser implementation or http_parser binding. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants