Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

node-api: remove RefBase and CallbackWrapper #53590

Merged
merged 4 commits into from
Aug 27, 2024
Merged

node-api: remove RefBase and CallbackWrapper #53590

merged 4 commits into from
Aug 27, 2024
+360 −350

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Jun 25, 2024 •
edited
Loading

This PR simplifies internal Node-API implementation to reduce the code complexity and to reduce allocated memory size.
There are no API changes or changes to the code behavior. All existing tests are passing and no new tests are needed.

  • Removed RefBase class.
    • Reference class directly inherits from RefTracker.
    • napi_set_instance_data and napi_get_instance_data use TrackedFinalizer instead of RefBase.
  • v8impl::Ownership enum is renamed to v8impl::ReferenceOwnership to better reflect its purpose.
    • v8impl::ReferenceOwnership size changed to one byte to reduce Reference size.
  • Finalizer class became non-virtual to reduce its size.
    • All Finalizer fields became private for better encapsulation.
    • TrackedFinalizer and TrackedStringResource size changed from 64 to 56 bytes on 64-bit platforms.
  • Reference class is split up into three classes depending on the usage scenario: Reference, ReferenceWithData, and ReferenceWithFinalizer.
    • Previously Reference instance size on 64-bit platforms was 88 bytes.
    • New Reference size is 40 bytes, ReferenceWithData is 48 bytes, and ReferenceWithFinalizer is 72 bytes.
    • ReferenceWithFinalizer size reduced by 16 bytes due to changes to Finalizer (8 bytes) and removal of RefBase that along with the v8impl::ReferenceOwnership to be one byte gained reduction of another 8 bytes.
    • Instances of Reference and ReferenceWithData are not queued for finalization since they do not have user finalizer callbacks. They are finalized immediately.
  • Simplified FunctionCallbackWrapper.
    • Removed base classes CallbackWrapper and CallbackWrapperBase
    • All methods became non-virtual.
    • It should slightly improve perf for each function call.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @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 25, 2024
legendecas
legendecas reviewed Jun 27, 2024
View reviewed changes
src/js_native_api_v8.cc Outdated Show resolved Hide resolved
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 27, 2024
@nodejs-github-bot
Copy link
Collaborator

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

legendecas
legendecas reviewed Jun 28, 2024
View reviewed changes
src/node_api.cc Outdated Show resolved Hide resolved
gabrielschulhof
gabrielschulhof approved these changes Jun 28, 2024
View reviewed changes
src/js_native_api_v8.cc Show resolved Hide resolved
src/js_native_api_v8.cc Show resolved Hide resolved
src/js_native_api_v8.h Outdated Show resolved Hide resolved
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2024
toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Jul 2, 2024
@toyobayashi
refactor: remove RefBase (nodejs/node#53590)
f08c821
@toyobayashi toyobayashi mentioned this pull request Jul 2, 2024
Merged
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@vmoroz vmoroz added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2024
@nodejs-github-bot
Copy link
Collaborator

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

mhdawson
mhdawson approved these changes Jul 12, 2024
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 12, 2024
@vmoroz vmoroz added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

anonrig
anonrig approved these changes Aug 14, 2024
View reviewed changes
src/js_native_api_v8.h Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@vmoroz @anonrig
add const for refcount()
2fceb77 
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
@legendecas legendecas added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@vmoroz vmoroz added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 25, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/53590
✔  Done loading data for nodejs/node/pull/53590
----------------------------------- PR info ------------------------------------
Title      node-api: remove RefBase and CallbackWrapper (#53590)
Author     Vladimir Morozov <vmorozov@microsoft.com> (@vmoroz)
Branch     vmoroz:PR/remove_refbase -> nodejs:main
Labels     c++, node-api, author ready, needs-ci
Commits    4
 - node-api: remove RefBase and CallbackWrapper
 - reduce size of Reference
 - address PR feedback
 - add const for refcount()
Committers 2
 - Vladimir Morozov <vmorozov@microsoft.com>
 - GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/53590
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53590
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - node-api: remove RefBase and CallbackWrapper
   ⚠  - reduce size of Reference
   ⚠  - address PR feedback
   ⚠  - add const for refcount()
   ℹ  This PR was created on Tue, 25 Jun 2024 21:35:58 GMT
   ✔  Approvals: 5
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/53590#pullrequestreview-2141773353
   ✔  - Gabriel Schulhof (@gabrielschulhof): https://github.com/nodejs/node/pull/53590#pullrequestreview-2148413989
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/53590#pullrequestreview-2151082401
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53590#pullrequestreview-2157911770
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/53590#pullrequestreview-2175228053
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-23T13:20:52Z: https://ci.nodejs.org/job/node-test-pull-request/61377/
- Querying data for job/node-test-pull-request/61377/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/10548497690

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 25, 2024
@vmoroz
Copy link
Member Author

vmoroz commented Aug 25, 2024

@nodejs/node-api , could you please sign off the latest changes. The last CI build is succeeded and we must be able to merge the PR.

@vmoroz
Copy link
Member Author

vmoroz commented Aug 27, 2024

@mhdawson , could you please sign off the latest change?
The last change was only adding the const keyword to refcount() suggested by @anonrig.
The PR passed all the CI validation and is ready to merge.

@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 27, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 27, 2024
@nodejs-github-bot nodejs-github-bot merged commit 431ac16 into nodejs:main Aug 27, 2024
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 431ac16

toyobayashi added a commit to toyobayashi/emnapi that referenced this pull request Aug 27, 2024
@toyobayashi
refactor: remove RefBase (#122)
5ab92c7 
* refactor: remove RefBase (nodejs/node#53590)

* export ReferenceOwnership as runtime value
@vmoroz vmoroz deleted the PR/remove_refbase branch August 27, 2024 21:25
@vmoroz
Copy link
Member Author

vmoroz commented Aug 27, 2024

@legendecas , thank you for merging it! :)

RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@vmoroz @RafaelGSS
node-api: remove RefBase and CallbackWrapper
f4a819e 
PR-URL: #53590
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@vmoroz @RafaelGSS
node-api: remove RefBase and CallbackWrapper
3cd10a3 
PR-URL: #53590
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@anonrig anonrig anonrig approved these changes

@legendecas legendecas legendecas approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vmoroz @nodejs-github-bot @jasnell @gabrielschulhof @anonrig @legendecas @mhdawson