Skip to content

http: coalesce chunked writes during auto-corking - #64987

Closed
GetThatCookie wants to merge 5 commits into
nodejs:mainfrom
GetThatCookie:pr64980_write_path
Closed

http: coalesce chunked writes during auto-corking#64987
GetThatCookie wants to merge 5 commits into
nodejs:mainfrom
GetThatCookie:pr64980_write_path

Conversation

@GetThatCookie

Copy link
Copy Markdown
Contributor

This PR is one of four focused changes split out of #64980 following review
feedback.

The full context, rationale, related work, benchmarks, and validation details
are documented there.

The AI-assistance disclosure in #64980 applies to this split PR as well.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 3, 2026
@pimterry

pimterry commented Aug 4, 2026

Copy link
Copy Markdown
Member

I haven't reviewed this in full yet (thanks for breaking up #64980, but this PR is still massive) but I've done a quick skim.

For the chunk writing improvements (independent of corking) I don't think most of this is necessary. There is a big improvement here in combining HTTP chunks within the same tick, which is great, but most of the rest of the code seems to be managing merging low-level calls which we already do internally anyway.

Testing on node without these changes, repeated writes here are already just one writev call:

const http = require('http');
const net = require('net');

const origWritev = net.Socket.prototype._writev;
net.Socket.prototype._writev = function (chunks, cb) {
  if (this.trackWrites) {
    console.log(`_writev called with ${chunks.length} chunks on socket ${this.remoteAddress}:${this.remotePort}`);
  }
  return origWritev.call(this, chunks, cb);
};

const origWrite = net.Socket.prototype._write;
net.Socket.prototype._write = function (data, encoding, cb) {
  if (this.trackWrites) {
    console.log(`_write called with ${data.length} bytes on socket ${this.remoteAddress}:${this.remotePort}`);
  }
  return origWrite.call(this, data, encoding, cb);
};

const server = http.createServer((req, res) => {
  req.socket.trackWrites = true;
  for (let i = 0; i < 16; i++) res.write('x'.repeat(64));
  res.end();
});

server.listen(0, () => {
  http.get({ port: server.address().port }, (res) => {
    let bytes = 0;
    res.on('data', (d) => bytes += d.length);
    res.on('end', () => server.close());
  });
});

Shows a single writev of 16 chunks for the 16 separate res.write calls. We should optimize the chunks, but we don't need to optimize the writev calls - it's already batched anyway.

Can you try simplifying this to combine chunked writes (in terms of transfer-encoding chunks I mean) but without all the writev internal stream changes? I think you'll find you get roughly identical perf boost with 10% of the code changes.

That's separate from the cork fix, I'll look at that closer but it would be helpful to clean this up first so the remaining logic is easier to review.

@GetThatCookie GetThatCookie changed the title http,net,stream: optimize outgoing write paths http: coalesce chunked writes during auto-corking Aug 4, 2026
@GetThatCookie

Copy link
Copy Markdown
Contributor Author

Thanks @pimterry you were right. I got a bit overenthusiastic here because there was a lot of room to explore, and I ended up trying to improve too many layers at once.

I now reduced the PR to the HTTP chunk coalescing and corking changes and removed the lower-level stream/writev/vector work. This makes both the invariant and the review surface much clearer.

The vector work was not performance-neutral in isolated measurements; it was typically around 15–20% faster across the configurations I tested. A single writev still leaves some overhead in constructing and processing the individual writes. However, that does not outweigh the additional complexity here. The major end-to-end benefit comes from combining the transfer-encoding chunks, and that can be retained with a fraction of the changes.

PS: Sorry, but I've also realigned the other PRs.

@ronag

ronag commented Aug 6, 2026

Copy link
Copy Markdown
Member

This feels a bit overlay complicated for what it tries to do. Any chance to consider a simpler solution?

@GetThatCookie

Copy link
Copy Markdown
Contributor Author

Simplified while retaining the cork/uncork fix and same-tick chunk coalescing - this should be the bare minimum.

Comment thread lib/_http_outgoing.js Outdated
Comment thread lib/_http_outgoing.js
Signed-off-by: GetThatCookie <NimmenKeks@gmx.de>
Signed-off-by: GetThatCookie <NimmenKeks@gmx.de>
Signed-off-by: GetThatCookie <NimmenKeks@gmx.de>
@GetThatCookie

Copy link
Copy Markdown
Contributor Author

@ronag Sorry, unfortunate timing β€” I had one final lint fix pending. I’m done now.

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.07%. Comparing base (f2c7f1b) to head (8c276ca).
⚠️ Report is 70 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64987      +/-   ##
==========================================
- Coverage   90.14%   90.07%   -0.08%     
==========================================
  Files         751      751              
  Lines      253648   254457     +809     
  Branches    47772    47993     +221     
==========================================
+ Hits       228643   229190     +547     
- Misses      16258    16464     +206     
- Partials     8747     8803      +56     
Files with missing lines Coverage Ξ”
lib/_http_outgoing.js 97.90% <100.00%> (+0.13%) ⬆️

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

@GetThatCookie

Copy link
Copy Markdown
Contributor Author

@ronag @pimterry as far as I'm concerned, the PR is now final - thank you for reviewing.

@trivikr trivikr added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Aug 26, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2026
@nodejs-github-bot

This comment was marked as resolved.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@trivikr trivikr added commit-queue-squash PRs the Commit Queue should land as one squashed commit. commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 28, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 28, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Commit Queue failed

   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
   ✘  GitHub CI is still running

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output
- Loading data for nodejs/node/pull/64987
βœ”  Done loading data for nodejs/node/pull/64987
----------------------------------- PR info ------------------------------------
Title      http: coalesce chunked writes during auto-corking (#64987)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     GetThatCookie:pr64980_write_path -> nodejs:main
Labels     lib / src, author ready, needs-ci, commit-queue, commit-queue-squash
Commits    5
 - http: coalesce chunked writes during auto-corking
 - http: simplify chunked write coalescing
 - http: preserve socket cork count during flush
 - http: fix lint error in writableLength
 - http: fix chunked write coalescing CI failures
Committers 1
 - GetThatCookie <NimmenKeks@gmx.de>
PR-URL: https://github.com/nodejs/node/pull/64987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
--------------------------------------------------------------------------------
   β„Ή  This PR was created on Mon, 03 Aug 2026 21:13:57 GMT
   βœ”  Approvals: 2
   βœ”  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/64987#pullrequestreview-5029956862
   βœ”  - Tim Perry (@pimterry): https://github.com/nodejs/node/pull/64987#pullrequestreview-5039877110
   ✘  GitHub CI is still running
   β„Ή  Last Full PR CI on 2026-08-28T06:41:55Z: https://ci.nodejs.org/job/node-test-pull-request/76618/
- Querying data for job/node-test-pull-request/76618/
βœ”  Build data downloaded
   βœ”  Last Jenkins CI successful
--------------------------------------------------------------------------------
   βœ”  Aborted `git node land` session in /home/runner/work/node/node/.ncu

View workflow run

@trivikr trivikr removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Aug 28, 2026
trivikr pushed a commit that referenced this pull request Aug 28, 2026
Signed-off-by: GetThatCookie <NimmenKeks@gmx.de>
PR-URL: #64987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
@trivikr

trivikr commented Aug 28, 2026

Copy link
Copy Markdown
Member

Landed in a382c1c

@trivikr trivikr closed this Aug 28, 2026
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. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants