Skip to content

node-api: provide napi_define_properties fast path#48440

Closed
gabrielschulhof wants to merge 1 commit intonodejs:mainfrom
gabrielschulhof:45905-object-creation-speedup
Closed

node-api: provide napi_define_properties fast path#48440
gabrielschulhof wants to merge 1 commit intonodejs:mainfrom
gabrielschulhof:45905-object-creation-speedup

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Jun 13, 2023 •
edited
Loading

Implement defining properties via V8's
v8::Object::CreateDataProperty(), which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: #45905

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 13, 2023

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Jun 13, 2023
@gabrielschulhof gabrielschulhof added performance Issues and PRs related to the performance of Node.js. and removed c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 13, 2023
@gabrielschulhof gabrielschulhof force-pushed the 45905-object-creation-speedup branch 2 times, most recently from e9a4032 to d8299a4 Compare June 13, 2023 04:56
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Jun 13, 2023 

                                                      confidence improvement accuracy (*)   (**)  (***)
napi/define_properties implem='runFastPath' n=5000000        ***     36.49 %       ±3.13% ±4.21% ±5.56%
napi/define_properties implem='runSlowPath' n=5000000          *     -2.82 %       ±2.53% ±3.39% ±4.43%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@gabrielschulhof gabrielschulhof force-pushed the 45905-object-creation-speedup branch from d8299a4 to 9e08c8b Compare June 13, 2023 14:36
@gabrielschulhof
node-api: provide napi_define_properties fast path
4049162 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: nodejs#45905
Signed-off-by: Gabriel Schulhof <Gschulhof@auction.com>
@gabrielschulhof gabrielschulhof force-pushed the 45905-object-creation-speedup branch from 9e08c8b to 4049162 Compare June 14, 2023 05:10
@gabrielschulhof gabrielschulhof added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 14, 2023
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 14, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 14, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52258/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 15, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52259/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 15, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/52260/

legendecas
legendecas approved these changes Jun 15, 2023
View reviewed changes
src/js_native_api_v8.cc

auto define_maybe =
obj->DefineProperty(context, property_name, descriptor);
defined_successfully = define_maybe.FromMaybe(false);
Copy link
Member

@legendecas legendecas Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should distinguish the state that the DefineProperty operation is failed (e.g. when the object is frozen) or the DefineProperty operation is throwing (e.g. a throwing Proxy handler defineProperty) and returning a different napi_status. As this behavior is not introduced in this PR, I'll create a follow-up PR to do so.

gabrielschulhof added a commit that referenced this pull request Jun 16, 2023
@gabrielschulhof
node-api: provide napi_define_properties fast path
3c35cd4 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: #45905
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #48440
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Jun 16, 2023

Landed in 3c35cd4.

@github-actions github-actions bot mentioned this pull request Jun 21, 2023
Open
29 tasks
@legendecas legendecas mentioned this pull request Jun 30, 2023
Closed
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@gabrielschulhof @RafaelGSS
node-api: provide napi_define_properties fast path
dbf9e9f 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: #45905
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #48440
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@gabrielschulhof @Ceres6
node-api: provide napi_define_properties fast path
71d8c32 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: nodejs#45905
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: nodejs#48440
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@gabrielschulhof @Ceres6
node-api: provide napi_define_properties fast path
507f74c 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: nodejs#45905
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: nodejs#48440
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 8, 2023
@gabrielschulhof @ruyadorno
node-api: provide napi_define_properties fast path
f5892b4 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: #45905
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #48440
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 8, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@gabrielschulhof @ruyadorno
node-api: provide napi_define_properties fast path
04dc090 
Implement defining properties via V8's
`v8::Object::CreateDataProperty()`, which is faster for data-valued,
writable, configurable, and enumerable properties.

Re: #45905
Signed-off-by: Gabriel Schulhof <gabrielschulhof@gmail.com>
PR-URL: #48440
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes

@tniessen tniessen tniessen approved these changes

@legendecas legendecas legendecas approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. performance Issues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gabrielschulhof @nodejs-github-bot @anonrig @tniessen @legendecas