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

lib: add options to the heap snapshot APIs #44989

Closed
wants to merge 5 commits into from

Conversation

joyeecheung
Copy link
Member

Support configuration of the HeapSnapshotMode and NumericsMode fields inf HeapSnapshotOptions in the JS APIs for heap snapshots.

@nodejs-github-bot 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 Oct 13, 2022
-->

* `options` {Object}
* `exposeInternals` {boolean} If true, expose internals in the heap snapshot.
Copy link
Member Author

@joyeecheung joyeecheung Oct 13, 2022

Choose a reason for hiding this comment

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

I am actually not sure if we should use constants (e.g. strings) for these, suggestions are welcome. Also AFAIK exposeInternals doesn't currently change anything for us because we don't use any embedder tracing mechanism provided by V8 that could would make a difference here, but this might change in the future.

doc/api/worker_threads.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
lib/internal/heap_utils.js Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
lib/internal/heap_utils.js Outdated Show resolved Hide resolved
Support configuration of the HeapSnapshotMode and NumericsMode
fields inf HeapSnapshotOptions in the JS APIs for heap snapshots.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

const {
exposeInternals = false,
exposeNumericValues = false,
} = options;
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use destructuring + default values because assignment doesn't work with kEmtpyObject. @jasnell @aduh95

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

joyeecheung added a commit that referenced this pull request Nov 8, 2022
Support configuration of the HeapSnapshotMode and NumericsMode
fields inf HeapSnapshotOptions in the JS APIs for heap snapshots.

PR-URL: #44989
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@joyeecheung
Copy link
Member Author

Landed in b872d30

@joyeecheung joyeecheung closed this Nov 8, 2022
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 9, 2022
Support configuration of the HeapSnapshotMode and NumericsMode
fields inf HeapSnapshotOptions in the JS APIs for heap snapshots.

PR-URL: nodejs#44989
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
Support configuration of the HeapSnapshotMode and NumericsMode
fields inf HeapSnapshotOptions in the JS APIs for heap snapshots.

PR-URL: #44989
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 10, 2022
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Dec 28, 2022
@danielleadams
Copy link
Contributor

@joyeecheung this broke when landing in v18.x-staging. Do you mind opening a backport PR for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants