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

v8: add setHeapSnapshotNearHeapLimit #44420

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Aug 27, 2022

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
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.

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
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
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
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
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
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
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

@nodejs-github-bot
Copy link
Collaborator

@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

Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
PR-URL: nodejs#44420
Refs: nodejs#33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 3, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@juanarbol juanarbol mentioned this pull request Oct 4, 2022
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 7, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
PR-URL: #44420
Refs: #33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
PR-URL: nodejs/node#44420
Refs: nodejs/node#33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
PR-URL: nodejs/node#44420
Refs: nodejs/node#33010
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@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++. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants