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.

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

src: re-implement the C++ WeakReference class with JS WeakRef #49053

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

test: make WeakReference tests robust

Previously we assume that the objects are GC'ed after one
global.gc() returns, which is not necessarily always the case. Use
gcUntil() to run GC multiple times if they are not GC'ed in the
first time around.

lib: implement WeakReference on top of JS WeakRef

The C++ implementation can now be done entirely in JS using WeakRef.
Re-implement it in JS instead to simplify the code.

src: remove C++ WeakReference implementation

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 7, 2023
@joyeecheung joyeecheung force-pushed the weakref branch 2 times, most recently from 07ea1d6 to d62bec6 Compare August 7, 2023 15:14
lib/internal/util.js Outdated Show resolved Hide resolved
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

That's awesome. I'm wondering if this should have an impact on memory performance. Possibly a good impact due to the removal of bridge communication?

@joyeecheung
Copy link
Member Author

I'm wondering if this should have an impact on memory performance

Reducing the number of GC roots can be a positive thing but I don't expect much difference in this regard, considering we don't normally create that many WeakReference internally anyway (unless the user uses domain heavily, but domain has long been deprecated)

@joyeecheung
Copy link
Member Author

Rebased to resolve conflict

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Previously we assume that the objects are GC'ed after one
global.gc() returns, which is not necessarily always the case. Use
gcUntil() to run GC multiple times if they are not GC'ed in the
first time around.
The C++ implementation can now be done entirely in JS using WeakRef.
Re-implement it in JS instead to simplify the code.
@joyeecheung
Copy link
Member Author

Rebased again to resolve conflict..

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Aug 16, 2023
Previously we assume that the objects are GC'ed after one
global.gc() returns, which is not necessarily always the case. Use
gcUntil() to run GC multiple times if they are not GC'ed in the
first time around.

PR-URL: #49053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
joyeecheung added a commit that referenced this pull request Aug 16, 2023
The C++ implementation can now be done entirely in JS using WeakRef.
Re-implement it in JS instead to simplify the code.

PR-URL: #49053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
joyeecheung added a commit that referenced this pull request Aug 16, 2023
PR-URL: #49053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@joyeecheung
Copy link
Member Author

Landed in 7215176...a49bc4b

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
Previously we assume that the objects are GC'ed after one
global.gc() returns, which is not necessarily always the case. Use
gcUntil() to run GC multiple times if they are not GC'ed in the
first time around.

PR-URL: #49053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
The C++ implementation can now be done entirely in JS using WeakRef.
Re-implement it in JS instead to simplify the code.

PR-URL: #49053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #49053
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants