Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61751 +/- ##
==========================================
- Coverage 89.73% 89.72% -0.01%
==========================================
Files 675 675
Lines 204525 204672 +147
Branches 39314 39320 +6
==========================================
+ Hits 183523 183645 +122
+ Misses 13300 13296 -4
- Partials 7702 7731 +29
🚀 New features to boost your workflow:
|
mcollina
force-pushed
the
http-parser-fast-api
branch
from
2934b6f to
a8e3a7f
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds V8 Fast API support to several HTTPParser instance methods in src/node_http_parser.cc to reduce argument marshaling overhead and improve HTTP client request body handling performance.
Changes:
- Added Fast API variants for
free,remove,pause,resume, andunconsume, with shared*Impl()helpers for slow/fast paths. - Registered Fast API methods via
SetFastMethod(...)and addedv8::CFunctionexternal references for snapshot compatibility. - Added Fast API call tracing via
TRACK_V8_FAST_API_CALL.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Since the Parser destructor isn't going to run the destroy() callbacks | ||
| // it needs to be triggered manually. | ||
| parser->EmitTraceEventDestroy(); | ||
| parser->EmitDestroy(); | ||
| } |
There was a problem hiding this comment.
FreeImpl() calls EmitTraceEventDestroy()/EmitDestroy(). AsyncWrap::EmitDestroy() can schedule SetImmediate / microtasks and touch V8 handles, which is not safe to run from a V8 Fast API callback. The fast path should either be removed for free, or FastFree should take FastApiCallbackOptions& and force a fallback to the slow callback whenever destroy hooks are enabled (or whenever it would call into V8).
| static void FastPause(Local<Object> receiver) { | ||
| if constexpr (should_pause) { | ||
| TRACK_V8_FAST_API_CALL("http_parser.pause"); | ||
| } else { | ||
| TRACK_V8_FAST_API_CALL("http_parser.resume"); |
There was a problem hiding this comment.
FastPause()/FastResume() bypass the context invariant enforced in the slow path (CHECK_EQ(env, parser->env())). That means cross-context usage would no longer be caught on the fast path, potentially masking bugs and diverging behavior. Consider adding a fast-path guard (e.g., obtain the current Environment* and CHECK_EQ, or use FastApiCallbackOptions& to fall back to the slow callback when the current context/Environment does not match).
Add V8 Fast API support for the following HTTP parser methods: - free(): triggers destroy callbacks - remove(): removes parser from connections list - pause(): pauses the HTTP parser - resume(): resumes the HTTP parser - unconsume(): removes parser as stream listener The following methods were intentionally not converted: - close(): deletes the parser object, unsafe in Fast API - consume(): requires unwrapping StreamBase from JS object This improves performance by bypassing V8's argument marshaling overhead for these frequently called methods.
mcollina
force-pushed
the
http-parser-fast-api
branch
from
a8e3a7f to
b83b335
Compare
gurgunday
left a comment
gurgunday
left a comment
There was a problem hiding this comment.
Looks great! I tried doing this a few months ago and didn't get any performance improvements. But maybe I did something wrong. The code LGTM.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
github-actions
bot
removed
the
request-ci
10 participants
Summary
free,remove,pause,resume, andunconsumeImplementation Details
*Impl()helper functions to share logic between slow and fast pathsTRACK_V8_FAST_API_CALLfor tracingSetFastMethodNote:
Close()andConsume()cannot use Fast API because:Close()deletes the parser object (unsafe in fast path)Consume()requires unwrappingStreamBasefrom JS objectBenchmark Results (10 runs, statistically significant)
client-request-body.js method='end' len=1024 type='buf'client-request-body.js method='write' len=256 type='utf'client-request-body.js method='write' len=256 type='buf'client-request-body.js method='end' len=256 type='buf'headers.js group='fewHeaders' len=5 n=10simple.js chunkedEnc=0 c=50 chunks=1 len=4 type='buffer'Test plan