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.

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

v8: add setHeapSnapshotNearHeapLimit #44420

Merged
merged 1 commit into from
Sep 8, 2022
Merged

v8: add setHeapSnapshotNearHeapLimit #44420

merged 1 commit into from
Sep 8, 2022

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Aug 27, 2022 •
edited
Loading

Add setHeapSnapshotNearHeapLimit method so users(such as APM) can set the limit at runtime.
Refs: #33010

cc @joyeecheung

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@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. v8 engine Issues and PRs related to the V8 dependency. labels Aug 27, 2022
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch from cde811c to 42855cd Compare August 27, 2022 17:36
joyeecheung
joyeecheung reviewed Aug 27, 2022
View reviewed changes
doc/api/v8.md Outdated Show resolved Hide resolved
src/node_v8.cc Outdated Show resolved Hide resolved
test/pummel/test-heapsnapshot-near-heap-limit-by-api.js Outdated Show resolved Hide resolved
test/pummel/test-heapsnapshot-near-heap-limit-by-api.js Outdated Show resolved Hide resolved
test/pummel/test-heapsnapshot-near-heap-limit-by-api.js Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch 2 times, most recently from 6570a10 to 2a56d7e Compare August 27, 2022 21:53
@theanarkh
Copy link
Contributor Author

I have updated the code, please help review again, thanks.

legendecas
legendecas reviewed Aug 28, 2022
View reviewed changes
lib/v8.js Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
doc/api/v8.md Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch 3 times, most recently from 0dc219e to 3e2e8ae Compare August 28, 2022 09:57
jasnell
jasnell reviewed Aug 28, 2022
View reviewed changes
doc/api/v8.md Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch from 3e2e8ae to 7a8e073 Compare August 28, 2022 14:34
legendecas
legendecas reviewed Aug 29, 2022
View reviewed changes
src/node_v8.cc Outdated Show resolved Hide resolved
lib/v8.js Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch 2 times, most recently from ec64ac8 to b77352d Compare August 31, 2022 14:41
joyeecheung
joyeecheung reviewed Aug 31, 2022
View reviewed changes
src/node_v8.cc Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch 3 times, most recently from ad9581a to b13c99c Compare August 31, 2022 19:58
joyeecheung
joyeecheung reviewed Sep 5, 2022
View reviewed changes
src/env.cc Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
lib/v8.js Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
lib/v8.js Outdated Show resolved Hide resolved
lib/v8.js Outdated Show resolved Hide resolved
doc/api/v8.md Show resolved Hide resolved
joyeecheung
joyeecheung reviewed Sep 5, 2022
View reviewed changes
src/node_v8.cc Outdated Show resolved Hide resolved
src/node_v8.cc Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch 3 times, most recently from 1c83fa5 to 14656e7 Compare September 5, 2022 16:21
@theanarkh
Copy link
Contributor Author

I have updated the code, please help review again, thanks !

@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch from 14656e7 to fa8d516 Compare September 5, 2022 16:41
joyeecheung
joyeecheung reviewed Sep 6, 2022
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Thanks for your patience! Mostly LGTM, some final comments.

src/env.cc Outdated Show resolved Hide resolved
test/pummel/test-heapsnapshot-near-heap-limit-by-api.js Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch from fa8d516 to c086602 Compare September 6, 2022 15:48
@theanarkh theanarkh force-pushed the set_near_heap_limit_callback_on_runtime branch from 2604a72 to 69fff46 Compare September 6, 2022 16:29
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 8, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 8, 2022
@nodejs-github-bot nodejs-github-bot merged commit e62f6ce into nodejs:main Sep 8, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in e62f6ce

This was referenced Sep 9, 2022
Open
Open
Open
Open
Open
Open
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@theanarkh @Fyko
v8: add setHeapSnapshotNearHeapLimit
25a05ce 
PR-URL: nodejs#44420
Refs: nodejs#33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@theanarkh @RafaelGSS
v8: add setHeapSnapshotNearHeapLimit
90e8977 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@theanarkh @RafaelGSS
v8: add setHeapSnapshotNearHeapLimit
d063a98 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@theanarkh @RafaelGSS
v8: add setHeapSnapshotNearHeapLimit
9aa6a56 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 3, 2022
@theanarkh @juanarbol
v8: add setHeapSnapshotNearHeapLimit
5324666 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
@theanarkh @juanarbol
v8: add setHeapSnapshotNearHeapLimit
97cf416 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
@juanarbol juanarbol mentioned this pull request Oct 4, 2022
Merged
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
@theanarkh @juanarbol
v8: add setHeapSnapshotNearHeapLimit
474d933 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 7, 2022
@theanarkh @juanarbol
v8: add setHeapSnapshotNearHeapLimit
1ab25e4 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
@theanarkh @juanarbol
v8: add setHeapSnapshotNearHeapLimit
01ccd27 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
@theanarkh @juanarbol
v8: add setHeapSnapshotNearHeapLimit
0beedb7 
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
@theanarkh @guangwong
v8: add setHeapSnapshotNearHeapLimit
a39dbc5 
PR-URL: nodejs/node#44420
Refs: nodejs/node#33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
@theanarkh @guangwong
v8: add setHeapSnapshotNearHeapLimit
0c5b425 
PR-URL: nodejs/node#44420
Refs: nodejs/node#33010
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@legendecas legendecas legendecas approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theanarkh @nodejs-github-bot @jasnell @joyeecheung @legendecas