src: track BaseObjects with an efficient list #55104

legendecas · 2024-09-24T16:11:00Z

Since BaseObjects are internal structs, use a linked list to efficiently
maintain the tracking list. This also makes iterating BaseObject list
efficient as it no longer needs to compare the BaseObject::DeleteMe
cleanup callback.

Refs: #54880

nodejs-github-bot · 2024-09-24T16:11:06Z

Review requested:

@nodejs/realm
@nodejs/startup

src/README.md

avivkeller · 2024-09-24T17:17:34Z

src/README.md

+libuv objects is performed automatically, i.e. handles are closed and requests
 are cancelled if possible.


Suggested change

libuv objects is performed automatically, i.e. handles are closed and requests

are cancelled if possible.

libuv objects is performed automatically, (i.e. handles are closed and requests

are cancelled, if possible).

Thanks for the suggestion but I don't think it is necessary to add parentheses here.

avivkeller · 2024-09-24T17:18:00Z

src/README.md

+#### Cleanup realms and BaseObjects
+
+Realm cleanup depends on the realm types. All realms are destroyed when the
+[`Environment`][] is destroyed with cleanup hook. A [`ShadowRealm`][] can also


Suggested change

[`Environment`][] is destroyed with cleanup hook. A [`ShadowRealm`][] can also

[`Environment`][] is destroyed with [a/the] cleanup hook. A [`ShadowRealm`][] can also

src/README.md

codecov · 2024-09-25T00:00:32Z

Codecov Report

❌ Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.24%. Comparing base (3c5ceff) to head (778da3f).
⚠️ Report is 2606 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_messaging.cc	28.57%	3 Missing and 2 partials ⚠️
src/node_realm-inl.h	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55104      +/-   ##
==========================================
- Coverage   88.24%   88.24%   -0.01%     
==========================================
  Files         651      651              
  Lines      183877   183867      -10     
  Branches    35858    35855       -3     
==========================================
- Hits       162266   162253      -13     
- Misses      14901    14904       +3     
  Partials     6710     6710

Files with missing lines	Coverage Δ
src/aliased_buffer-inl.h	`93.58% <ø> (ø)`
src/base_object.cc	`84.61% <100.00%> (+0.94%)`	⬆️
src/base_object.h	`90.90% <100.00%> (+2.02%)`	⬆️
src/cleanup_queue-inl.h	`90.90% <ø> (+3.40%)`	⬆️
src/cleanup_queue.h	`100.00% <100.00%> (ø)`
src/debug_utils-inl.h	`83.33% <ø> (ø)`
src/env.cc	`85.55% <100.00%> (-0.17%)`	⬇️
src/node_realm.cc	`73.41% <100.00%> (ø)`
src/node_realm.h	`100.00% <ø> (ø)`
src/node_shadow_realm.cc	`62.85% <100.00%> (ø)`
... and 3 more

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina

lgtm

src/node_realm.h

nodejs-github-bot · 2024-09-25T13:39:08Z

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

nodejs-github-bot · 2024-09-26T08:31:26Z

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

nodejs-github-bot · 2024-09-26T16:27:20Z

Commit Queue failed

- Loading data for nodejs/node/pull/55104
✔  Done loading data for nodejs/node/pull/55104
----------------------------------- PR info ------------------------------------
Title      src: track BaseObjects with an efficient list (#55104)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     legendecas:realm/base-obj -> nodejs:main
Labels     c++, lib / src, author ready, needs-ci
Commits    1
 - src: track BaseObjects with an efficient list
Committers 1
 - Chengzhong Wu <legendecas@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/55104
Refs: https://github.com/nodejs/node/pull/54880
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55104
Refs: https://github.com/nodejs/node/pull/54880
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 24 Sep 2024 16:11:00 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55104#pullrequestreview-2327733103
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/55104#pullrequestreview-2328639766
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-09-26T08:31:26Z: https://ci.nodejs.org/job/node-test-pull-request/62782/
- Querying data for job/node-test-pull-request/62782/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/11056331072

PR-URL: #55104 Refs: #54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

legendecas · 2024-09-26T17:41:32Z

Landed in 8496670

PR-URL: #55104 Refs: #54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos · 2024-10-04T17:22:12Z

I had to remove from v22.x-staging because it failed in GitHub CI (I think I didn't catch it because make test doesn't run pummel tests?):
https://github.com/nodejs/node/actions/runs/11179405352/job/31078955626

PR-URL: nodejs#55104 Refs: nodejs#54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #55104 Backport-PR-URL: #59959 Refs: #54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Sep 24, 2024

legendecas marked this pull request as draft September 24, 2024 16:25

avivkeller reviewed Sep 24, 2024

View reviewed changes

src/README.md Outdated Show resolved Hide resolved

avivkeller reviewed Sep 24, 2024

View reviewed changes

src: track BaseObjects with an efficient list

778da3f

legendecas force-pushed the realm/base-obj branch from 74b8541 to 778da3f Compare September 24, 2024 22:39

legendecas marked this pull request as ready for review September 24, 2024 22:39

mcollina approved these changes Sep 25, 2024

View reviewed changes

joyeecheung reviewed Sep 25, 2024

View reviewed changes

src/node_realm.h Show resolved Hide resolved

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2024

joyeecheung approved these changes Sep 25, 2024

View reviewed changes

legendecas added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 26, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 26, 2024

legendecas added a commit that referenced this pull request Sep 26, 2024

src: track BaseObjects with an efficient list

8496670

PR-URL: #55104 Refs: #54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

legendecas closed this Sep 26, 2024

legendecas removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 26, 2024

legendecas deleted the realm/base-obj branch September 26, 2024 17:41

targos pushed a commit that referenced this pull request Oct 4, 2024

src: track BaseObjects with an efficient list

cceb802

PR-URL: #55104 Refs: #54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Oct 4, 2024

targos mentioned this pull request Oct 4, 2024

[v22.x backport] doc: update require(ESM) history and stability status #55263

Merged

aduh95 mentioned this pull request Oct 14, 2024

2024-10-16, Version 23.0.0 (Current) #55338

Merged

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

src: track BaseObjects with an efficient list

4fd5cdf

PR-URL: nodejs#55104 Refs: nodejs#54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

src: track BaseObjects with an efficient list

e68a566

PR-URL: nodejs#55104 Refs: nodejs#54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

legendecas mentioned this pull request Sep 21, 2025

[v22.x] src: track BaseObjects with an efficient list #59959

Closed

legendecas added backport-open-v22.x Indicate that the PR has an open backport and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Sep 21, 2025

legendecas added a commit to legendecas/node that referenced this pull request Sep 21, 2025

src: track BaseObjects with an efficient list

8b3a537

PR-URL: nodejs#55104 Refs: nodejs#54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

richardlau pushed a commit that referenced this pull request Sep 22, 2025

src: track BaseObjects with an efficient list

937e9bb

PR-URL: #55104 Backport-PR-URL: #59959 Refs: #54880 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

github-actions bot mentioned this pull request Sep 22, 2025

2025-09-24, Version 22.20.0 'Jod' (LTS) #59973

Merged

richardlau added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Sep 24, 2025

		libuv objects is performed automatically, i.e. handles are closed and requests
		are cancelled if possible.

	[`Environment`][] is destroyed with cleanup hook. A [`ShadowRealm`][] can also
	[`Environment`][] is destroyed with [a/the] cleanup hook. A [`ShadowRealm`][] can also

Uh oh!

src: track BaseObjects with an efficient list #55104

src: track BaseObjects with an efficient list #55104

Uh oh!

Conversation

legendecas commented Sep 24, 2024

Uh oh!

nodejs-github-bot commented Sep 24, 2024

Uh oh!

Uh oh!

avivkeller Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

legendecas Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

avivkeller Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 25, 2024

Uh oh!

nodejs-github-bot commented Sep 26, 2024

Uh oh!

nodejs-github-bot commented Sep 26, 2024

Uh oh!

legendecas commented Sep 26, 2024

Uh oh!

targos commented Oct 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Sep 25, 2024 •

edited

Loading