Skip to content

lib: add options to the heap snapshot APIs#44989

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
Closed
joyeecheung wants to merge 5 commits into from
Closed

lib: add options to the heap snapshot APIs #44989

joyeecheung wants to merge 5 commits into from

Conversation

@joyeecheung
Copy link
Member

@joyeecheungjoyeecheung commented Oct 13, 2022

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

@nodejs-github-botnodejs-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
joyeecheung
joyeecheung commented Oct 13, 2022
View reviewed changes
doc/api/v8.md
-->

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

@joyeecheungjoyeecheungOct 13, 2022•
edited
Loading

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.

joyeecheung
joyeecheung commented Oct 13, 2022
View reviewed changes
aduh95
aduh95 reviewed Oct 13, 2022
View reviewed changes
aduh95
aduh95 reviewed Oct 13, 2022
View reviewed changes
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
@joyeecheung
lib: add options to the heap snapshot APIs
df587a0
Support configuration of the HeapSnapshotMode and NumericsMode fields inf HeapSnapshotOptions in the JS APIs for heap snapshots.
@joyeecheung
fixup! lib: add options to the heap snapshot APIs
cbe5987
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 31, 2022

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 31, 2022

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

@github-actionsgithub-actionsbot mentioned this pull request Nov 1, 2022
Open
17 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 1, 2022

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

joyeecheung
joyeecheung commented Nov 1, 2022
View reviewed changes
lib/internal/heap_utils.js
const{
exposeInternals =false,
exposeNumericValues =false,
}=options;
Copy link
MemberAuthor

@joyeecheungjoyeecheungNov 1, 2022

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 commented Nov 1, 2022

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

@github-actionsgithub-actionsbot mentioned this pull request Nov 2, 2022
Open
15 tasks
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 7, 2022

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 7, 2022

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

@github-actionsgithub-actionsbot mentioned this pull request Nov 8, 2022
Open
17 tasks
joyeecheung added a commit that referenced this pull request Nov 8, 2022
@joyeecheung
lib: add options to the heap snapshot APIs
b872d30
Support configuration of the HeapSnapshotMode and NumericsMode fields inf HeapSnapshotOptions in the JS APIs for heap snapshots. PR-URL: #44989 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
@joyeecheung
Copy link
MemberAuthor

joyeecheung commented Nov 8, 2022

Landed in b872d30

@github-actionsgithub-actionsbot mentioned this pull request Nov 9, 2022
Open
14 tasks
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 9, 2022
@joyeecheung
lib: add options to the heap snapshot APIs
2021876
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 <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
@github-actionsgithub-actionsbot mentioned this pull request Nov 10, 2022
Open
20 tasks
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@joyeecheung@RafaelGSS
lib: add options to the heap snapshot APIs
0478e40
Support configuration of the HeapSnapshotMode and NumericsMode fields inf HeapSnapshotOptions in the JS APIs for heap snapshots. PR-URL: #44989 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Nov 10, 2022
Merged
@github-actionsgithub-actionsbot mentioned this pull request Nov 11, 2022
Open
19 tasks
@danielleadams
Copy link
Contributor

danielleadams commented Dec 28, 2022

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments

@aduh95aduh95aduh95 left review comments

@addaleaxaddaleaxaddaleax approved these changes

@legendecaslegendecaslegendecas approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@joyeecheung@nodejs-github-bot@danielleadams@jasnell@addaleax@legendecas@aduh95