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: implement GetDetachedness() in MemoryRetainerNode #44803

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Sep 27, 2022

This allows us to mark weak/detached references in the heap snapshot. Also mark weak/detached BaseObject with Detachedness::kDetached so that the state of the reference can be displayed by frontend consuming the heap snapshot.

With the following snippet:

const dns = require('dns');
dns.resolve('localhost', () => {
  setTimeout(write, 1000);
});

function write() {
  const v8 = require('v8');
  v8.writeHeapSnapshot();
}

The cleanup queue in the generated heap snapshot previously looked like this in the DevTools - nodes that will be GC'ed once they are no longer referenced from JS land are not distinguished from nodes that are kept alive from the C++ land.

Screen Shot 2022-09-27 at 15 13 43

Now it looks like this - nodes that will be GC'ed once they are no longer referenced from JS have the Detached label (note that in the case of ChannelWrap, if we take the snapshot before the query completes, the ChannelWrap won't be marked as Detached, because it would still be reachable from the QueryWrap which is kept alive by C++ i.e. being attached - objects reachable from attached objects are also considered attached by DevTools):

Screen Shot 2022-09-27 at 15 13 49

This allows us to mark weak/detached references in the heap snapshot.
Also mark weak/detached BaseObject with Detachedness::kDetached
so that the state of the reference can be displayed by frontend
consuming the heap snapshot.
@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. labels Sep 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM but linter is complaining.

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the review wanted PRs that need reviews. label Sep 29, 2022
@joyeecheung joyeecheung removed the review wanted PRs that need reviews. label Oct 3, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung 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. labels Oct 5, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/44803
✔  Done loading data for nodejs/node/pull/44803
----------------------------------- PR info ------------------------------------
Title      src: implement GetDetachedness() in MemoryRetainerNode (#44803)
Author     Joyee Cheung  (@joyeecheung)
Branch     joyeecheung:detached-retainer -> nodejs:main
Labels     c++, needs-ci, commit-queue-squash
Commits    2
 - src: implement GetDetachedness() in MemoryRetainerNode
 - fixup! src: implement GetDetachedness() in MemoryRetainerNode
Committers 1
 - Joyee Cheung 
PR-URL: https://github.com/nodejs/node/pull/44803
Reviewed-By: Chengzhong Wu 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44803
Reviewed-By: Chengzhong Wu 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 27 Sep 2022 07:24:01 GMT
   ✔  Approvals: 2
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/44803#pullrequestreview-1124920321
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/44803#pullrequestreview-1127622249
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-10-04T17:02:48Z: https://ci.nodejs.org/job/node-test-pull-request/47071/
- Querying data for job/node-test-pull-request/47071/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3189452691

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 5, 2022
joyeecheung added a commit that referenced this pull request Oct 5, 2022
This allows us to mark weak/detached references in the heap snapshot.
Also mark weak/detached BaseObject with Detachedness::kDetached
so that the state of the reference can be displayed by frontend
consuming the heap snapshot.

PR-URL: #44803
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung closed this Oct 5, 2022
danielleadams pushed a commit that referenced this pull request Oct 11, 2022
This allows us to mark weak/detached references in the heap snapshot.
Also mark weak/detached BaseObject with Detachedness::kDetached
so that the state of the reference can be displayed by frontend
consuming the heap snapshot.

PR-URL: #44803
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed An error occurred while landing this 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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants