src: add MakeCallback() taking an Environment - #66510
Draft
nigrosimone wants to merge 2 commits into
Draft
nigrosimone wants to merge 2 commits into
nigrosimone wants to merge 2 commits into
Conversation
Without an async context, napi_make_callback went through the public node::MakeCallback(), which looks up the Environment from the creation context of the callback on every call. The napi_env already knows it, so call into it directly, as the path with an async context does. node_napi_env__::node_env() also looked it up from the context on every call: find it once, a napi_env lives as long as its Environment. Add a benchmark, napi/make_callback_napi. Refs: nodejs/performance#24 Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
node::MakeCallback() looks up the Environment from the creation context of the callback on every call. An addon that calls into the same Environment many times, a server calling one handler per request for example, can now take it once with GetCurrentEnvironment() and pass it. The callback must belong to that Environment. Add a benchmark, napi/make_callback_env, comparing the two overloads. Refs: nodejs/performance#24 Signed-off-by: Nigro Simone <nigro.simone@gmail.com>
Collaborator
|
Review requested:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
node::MakeCallback()looks up the Environment from the creation context of the callback on every call. This adds an overload that takes it: an addon calling into the same Environment many times (uWS calls one handler per request) takes it once withGetCurrentEnvironment()and passes it. The callback must belong to that Environment.On top of #66509 (the first commit is that PR), which adds the internal helper both use. The new benchmark
napi/make_callback_envruns both overloads in the same process: 133.0 to 118.9 ns at n=1e7 (+11.8%) and 130.6 to 124.5 ns at n=1e6 (+4.9%), with #66395 and #66500 underneath, Linux x64, one core.Draft because this is new public API: the shape is open, see nodejs/performance#24.
Disclosure: I used Opus 5.5 (Max) as coding assistant