Skip to content

src: be less conservative in NearHeapLimitCallback - #50718

Closed
joyeecheung wants to merge 1 commit into
nodejs:mainfrom
joyeecheung:near-heap-limit-limit
Closed

joyeecheung wants to merge 1 commit into
nodejs:mainfrom
joyeecheung:near-heap-limit-limit

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Nov 13, 2023 •

Copy link
Copy Markdown
Member

Previously the callback only considered potential overhead coming from promotion. As it turns out the cache for calculated line ends could also increase heap memory usage during heap snapshot generation. This patch makes the raised limit less conservative using a formula of (young_gen_size + old_gen_size / 2) for the extra leeway given to the heap limit.

Drive-by: use uv_get_available_memory() to calculate the memory available to the process directly and print the memory in MB in the debugging logs.

Refs: #50711

@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 Nov 13, 2023
Comment thread src/env.cc Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure about this formula - maybe we should use inital_heap_limit here as the base instead of avoid unbound growth of the new limit

Previously the callback only considered potential overhead coming
from promotion. As it turns out the cache for calculated line ends
could also increase heap memory usage during heap snapshot
generation. This patch makes the raised limit less conservative
using a formula of (young_gen_size + old_gen_size / 2) for the
extra leeway given to the heap limit.

Drive-by: use uv_get_available_memory() to calculate the memory
available to the process directly and print the memory in MB
in the debugging logs.
@joyeecheung
joyeecheung force-pushed the near-heap-limit-limit branch from e9e6170 to a6616b1 Compare November 14, 2023 00:47
@joyeecheung

joyeecheung commented Jun 5, 2024 •

Copy link
Copy Markdown
Member Author

I believe this is no longer necessary after the changes in v8 https://issues.chromium.org/issues/42204564 that ensures the heap snapshot generation/serialization only use off-heap memory for the heap snapshot itself (need to wait until v8 12.5 update to roll it in though).

@joyeecheung joyeecheung closed this Jun 5, 2024
@joyeecheung

Copy link
Copy Markdown
Member Author

Actually on macOS, #50711 is still skipped, because of libuv/libuv#3897

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants