src: improve performance of http parser #58288
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
anonrig
force-pushed
the
yagiz/optimize-http-parser
branch
from
f0f2213 to
6ba2682
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58288 +/- ##
=======================================
Coverage 90.18% 90.18%
=======================================
Files 631 631
Lines 186690 186741 +51
Branches 36666 36661 -5
=======================================
+ Hits 168360 168415 +55
+ Misses 11126 11124 -2
+ Partials 7204 7202 -2
🚀 New features to boost your workflow:
|
| @@ -17,6 +17,11 @@ using CFunctionCallbackWithMultipleValueAndOptions = | |||
| v8::Local<v8::Value>, | |||
| v8::Local<v8::Value>, | |||
| v8::FastApiCallbackOptions&); | |||
| using CFunctionVoid = void (*)(v8::Local<v8::Value>); | |||
| using CFunctionVoid2 = | |||
There was a problem hiding this comment.
Not super excited about the name here but then again none of the macro names here are great.
| Local<Value>(), | ||
| signature, | ||
| 0, | ||
| v8::ConstructorBehavior::kThrow, |
There was a problem hiding this comment.
using v8::ConstructorBehavior at the top of the file
| c_function); | ||
| // kInternalized strings are created in the old space. | ||
| const v8::NewStringType type = v8::NewStringType::kInternalized; | ||
| Local<v8::String> name_string = |
| @@ -711,7 +739,6 @@ class Parser : public AsyncWrap, public StreamListener { | |||
| } | |||
| } | |||
|
|
|||
| // TODO(@anonrig): Add V8 Fast API | |||
| static void Consume(const FunctionCallbackInfo<Value>& args) { | |||
|
Benchmark CI results are available at: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1717/consoleText I think there is something fundamentally wrong with either v8 fast api, or with our benchmarks. These changes shouldn't show a 11% performance degradation. @H4ad will run fastify benchmarks on this pull-request today. it might show a different result... cc @nodejs/performance @nodejs/v8 |
It looks like one of the many cases where using fast API does not make sense, as already explained in #58080 (comment) - getting the Environment requires looking it up from the current context which gets passed through a local handle which needs a handle scope. We should probably restrict the uses of fast API to bindings that do not need the handle scope (and by extension, do not need the Environment). |
|
@joyeecheung since we have permission checks in almost all functions, it's impossible to add v8 fast api to any more methods then. I think we should change how environment::getcurrent isolate creates handle scope, and only do it when it's needed. For nodejs, it seems v8 fast API is almost useless even for basic functions. |
|
In deno we store the "environment" reference in a |
|
I run the fastify-benchmarks, and to summarize I don't think the data was good enough to mark this PR as an improvement: ResultsNew node is this PR, current-main is with the revert-commit of this PR.
Results (with the JSON) in case someone wants to do some more analysis. I tried to run with little noise as possible on my setup (for fastify I tried to pin the CPU). For now, I would trust more the benchmarks we have for node and try the approach @devsnek suggested. |
I think permission checking is relatively rare outside of fs and module bindings? e.g. the APIs being touched here do not need that.
Actually I think many bindings don't necessarily even need |
|
Yes, I'll do a follow up for Environment::GetCurrent. Regarding this pull-request, since these changes in this pull-request are updating prototype methods, they only call this function, and I don't see any possible way to improve the performance using fast api with these methods. |
|
I got Environment while registering the function and passed it as a data argument for function template, but unfortunately it crashes due to Environment::GetCurrent() returning nullptr. Any idea why, and how to fix? @legendecas @joyeecheung @devsnek |
|
Could we replace this code with llhttp wasm similar to what Undici does? |
| @@ -174,6 +174,13 @@ inline bool TickInfo::has_rejection_to_warn() const { | |||
| return fields_[kHasRejectionToWarn] == 1; | |||
| } | |||
|
|
|||
| inline Environment* Environment::GetCurrent(v8::Local<v8::Value> value) { | |||
There was a problem hiding this comment.
Is the expectation that an v8::Localv8::Value instance allows us to retrieve an associated v8::Context?
There was a problem hiding this comment.
@lemire Take a look into: https://github.com/nodejs/node/compare/main...anonrig:node:yagiz/try-environment?expand=1
I'm trying to store the environment pointer as a v8::External in FunctionTemplate (just as @devsnek recommended) so that we can bypass HandleScope creation for getting Environment* in fast api calls.
v8::External extends from v8::Value. So, we can technically convert v8::Localv8::External (which is Environment*) to Environment*
There was a problem hiding this comment.
If you have a v8::Local (aka a handle), you must have a HandleScope
There was a problem hiding this comment.
If you have a
v8::Local(aka a handle), you must have a HandleScope
In this case, handle scope is created before the function callback is executed, and it's just a static cast since external value is void*. So, callback doesn't need an extra handle scope on fast api.
we can’t - and possibly we shouldn’t because we supporting running Node.js with wasm disabled. |
Isn't that already a problem with fetch? |
|
Yes, but that was a new implementation. Removing it would be semver-major. Would also prevent any program to create a server - making it useless. |
Adds v8 fast api to almost all possible http parser methods.
Pending benchmarks.
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1717/
cc @nodejs/performance @nodejs/http