http: improve http parser performance #59621

anonrig · 2025-08-25T16:51:12Z

Continues the work of @JonasBa on #57938

Improves the performance of http parser by 5% by removing the async_hooks on an synchronous http parser. I'm not sure why we had async_hooks in the first place. @mcollina was the first person who made me realize about this, so kudos to him for realizing this in unneeded.

nodejs-github-bot · 2025-08-25T16:51:18Z

Review requested:

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

Co-Authored-by: Jonas Badalic <[email protected]>

jasnell · 2025-08-25T16:59:35Z

src/async_wrap.h

@@ -49,8 +49,6 @@ namespace node {
  V(HTTP2STREAM)                                                               \
  V(HTTP2PING)                                                                 \
  V(HTTP2SETTINGS)                                                             \
-  V(HTTPINCOMINGMESSAGE)                                                       \
-  V(HTTPCLIENTREQUEST)                                                         \


Hmm.. it occurs to me now that we've never really considered if removing these constitutes a breakig (semver-major) change. This does, afterall, change the number that is associated with the other providers observable via require('async_hooks').asyncWrapProviders. I know that async hooks as a whole are still considered experimental but it's been around for so long that changes here could actually be breaking for some people. @nodejs/tsc .. thoughts?

With AsyncLocalStorage migrated away from async_hooks at this point, I don't think there's any significant remaining use of async_hooks, so I would not feel too worried about it. 🤔

In the past we didn't care about this. I know about at least one case where some internal refactoring caused a rename of an entry in this list.

jasnell · 2025-08-25T17:02:04Z

src/node_http_parser.cc

-        env()->context(), object(), 0, nullptr);
-
-      if (r.IsEmpty()) callback_scope.MarkAsFailed();
+      USE(cb.As<Function>()->Call(env()->context(), object(), 0, nullptr));


Is this actually correct for correct error propagation? @addaleax

Doesn't seem like it, no.

I honestly don't quite get what the idea behind this PR is. Maybe @anonrig or @mcollina can expand on this a bit more, and ideally incorporate it into the commit message, so that it's accessible not just for us as reviewers but also for future context. But as it stands, the correct title for the PR should be http: remove async tracking, not http: improve http parser performance.

mcollina · 2025-08-25T17:20:50Z

lib/_http_common.js

-    } else {
-      // Since the Parser destructor isn't going to run the destroy() callbacks
-      // it needs to be triggered manually.
-      parser.free();


can you investigate if you can avoid the setImmediate before?

I didn't dive deeper but removing it breaks the following 3 tests

out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/yagiz/coding/node/test/sequential/test-http-regr-gh-2928.js out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/yagiz/coding/node/test/parallel/test-http-upgrade-server2.js out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/yagiz/coding/node/test/parallel/test-set-http-max-http-headers.js

I think it really depends on the breakage and errors for those.

codecov · 2025-08-25T18:27:43Z

Codecov Report

❌ Patch coverage is 63.33333% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.81%. Comparing base (6722642) to head (05606e5).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http_parser.cc	63.33%	3 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59621      +/-   ##
==========================================
- Coverage   89.84%   89.81%   -0.03%     
==========================================
  Files         667      667              
  Lines      196260   196217      -43     
  Branches    38563    38553      -10     
==========================================
- Hits       176332   176238      -94     
- Misses      12382    12420      +38     
- Partials     7546     7559      +13

Files with missing lines	Coverage Δ
lib/_http_client.js	`97.31% <ø> (-0.03%)`	⬇️
lib/_http_common.js	`100.00% <ø> (ø)`
lib/_http_server.js	`96.93% <ø> (-0.10%)`	⬇️
src/node_http_parser.cc	`84.10% <63.33%> (-0.16%)`	⬇️

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

mcollina · 2025-08-25T20:19:41Z

@jasnell echoing what @Qard has said, I've marked it as dont-land on both 20.x and 22.x. I don't think there are problems in landing this on 24.

addaleax

I'm not sure why we had async_hooks in the first place.

We have the AsyncWrap integration, and more generally 'callback handling' logic (CallbackScope or MakeCallback()) for the same reason we have it anywhere else in Node.js -- when we are entering JS from C++ (without a JS call stack underneath), we need to take care of things like handling uncaught exceptions and running the microtask queue, and, well, async context tracking.

Minimal regression test for the type of bug that would be introduced by this PR:

'use strict';
const { get, createServer } = require('node:http');
const { executionAsyncId } = require('node:async_hooks');
const assert = require('node:assert');

createServer((req, res) => {
  assert.notStrictEqual(executionAsyncId(), 0); // this test makes this fail
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end('Hello, World!\n');
}).listen(0, function() {
  get(`http://localhost:${this.address().port}/`, () => this.close());
});

addaleax · 2025-08-25T20:35:32Z

src/node_http_parser.cc


-    if (r.IsEmpty())
-      got_exception_ = true;
+    if (try_catch.HasCaught()) got_exception_ = true;


What happens to these exceptions?

addaleax · 2025-08-25T20:37:11Z

src/node_http_parser.cc

-        env()->context(), object(), 0, nullptr);
-
-      if (r.IsEmpty()) callback_scope.MarkAsFailed();
+      USE(cb.As<Function>()->Call(env()->context(), object(), 0, nullptr));


Doesn't seem like it, no.

I honestly don't quite get what the idea behind this PR is. Maybe @anonrig or @mcollina can expand on this a bit more, and ideally incorporate it into the commit message, so that it's accessible not just for us as reviewers but also for future context. But as it stands, the correct title for the PR should be http: remove async tracking, not http: improve http parser performance.

addaleax · 2025-08-25T21:03:58Z

Also, just going to echo what @Flarna said in the original PR:

No, I haven't verified this. Basically this reverts fixes done a while ago (#27477 and #25094) to prepare HTTP for a better AsyncLocalStorage implementation.

Could be that AsyncContextFrame-backed AsyncLocalStorage solves this meanwhile, but it could be also that the move to AsyncContextFrame broke it already.
To my understanding AsyncContextFrame solution either follows promises/microtasks automatically inside v8, or it's told from outside. This happens for example in AsyncResource class on JS side or AsyncWrap class in C++. And exactly this AsyncWrap is removed here from HTTP parser.

In the end we don't know as there are no test (ref: #55712 - I thought I find some more time to add tests...).

I agree that the most meaningful start for making this sort of optimization is to ensure that we have the necessary test coverage before jumping to conclusions about what is and isn't feasible or acceptable.

Flarna · 2025-08-26T18:00:04Z

I'm not sure why we had async_hooks in the first place.

see corresponding comment in original PR:

While I agree that HTTP parser itself is not async HTTP requests are. As a result I'm fine to the change of http parser itself but the async hooks functionality itself should preserved on HTTP request level otherwise then.

Quite a while ago I tried such an approach to use the http.IncomingMessage instance as async resource instead the http parser. Background that time was to avoid the need for an asnc resource reset on the http parser because it's reused.
But it turned out to be not that trivial and I stopped this approach. Maybe someone with more experience in node source has more luck.

anonrig requested review from mcollina, jasnell and dario-piotrowicz August 25, 2025 16:51

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 25, 2025

anonrig requested a review from Qard August 25, 2025 16:53

http: improve http parser performance

05606e5

Co-Authored-by: Jonas Badalic <[email protected]>

anonrig force-pushed the yagiz/improve-http-parser-perf branch from 4d3aad7 to 05606e5 Compare August 25, 2025 16:53

anonrig requested a review from panva August 25, 2025 16:55

jasnell reviewed Aug 25, 2025

View reviewed changes

mcollina reviewed Aug 25, 2025

View reviewed changes

mcollina added dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Aug 25, 2025

addaleax requested changes Aug 25, 2025

View reviewed changes

Flarna added async_hooks Issues and PRs related to the async hooks subsystem. async_local_storage AsyncLocalStorage labels Aug 26, 2025

Uh oh!

http: improve http parser performance #59621

Are you sure you want to change the base?

http: improve http parser performance #59621

Uh oh!

Conversation

anonrig commented Aug 25, 2025

Uh oh!

nodejs-github-bot commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina commented Aug 25, 2025

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 25, 2025

Uh oh!

Flarna commented Aug 26, 2025

Uh oh!

Uh oh!

codecov bot commented Aug 25, 2025 •

edited

Loading