src: fix near heap limit callback #42636
Conversation
- Use the allocated space size to calculate the raised heap limit, as that is what V8 uses to determine whether it should crash - previously we use the used size for the calculation and that was too conservative and did not prevent the crashes effectively enough. - Use RequestInterrupt() to take the snapshot since we need to make sure that the heap limit is raised first before the snapshot can be taken.
nodejs-github-bot
added
c++
|
This fixes the crash locally for me, with or without the V8 update. I have another fix in V8 for this issue (https://chromium-review.googlesource.com/c/v8/v8/+/3575468) but when testing it I realized that we should just use RequestInterrupt to take the snapshot, because even when the assertions are lifted, the current behavior still relies on that V8 allows the callback to be nested so we have a chance to raise the limit when it's invoked again from the GC it triggers, which seems more complicated than necessary if we can just put the snapshot code in a RequestInterrupt callback and go ahead raising the limit the first time the NearHeapLimitCallback is triggered. |
| // space_size() returns the allocated size of a given space, | ||
| // we use this to calculate the new limit because V8 also | ||
| // uses the allocated size to determine whether it should crash. |
There was a problem hiding this comment.
| // space_size() returns the allocated size of a given space, | |
| // we use this to calculate the new limit because V8 also | |
| // uses the allocated size to determine whether it should crash. | |
| // space_size() returns the allocated size of a given space. | |
| // We use this to calculate the new limit because V8 also | |
| // uses the allocated size to determine whether it should crash. |
| uint64_t estimated_overhead = young_gen_size; | ||
| // The new limit must be higher than current_heap_limit or V8 might | ||
| // crash. | ||
| uint64_t minimun_new_limit = static_cast<uint64_t>(current_heap_limit + 1); |
|
@joyeecheung Are we waiting for the V8 patch instead of this PR? |
|
@targos If we land the V8 fix while still allowing nesting (i.e. the old behavior) then this can land later (the V8 fix alone should be enough to unblock the V8 update without this patch). If the V8 fix disallows nesting this has to be land first and v8/node-ci needs to pick this up to unbreak the V8 CI before the V8 patch can then land. Given that we want the V8 update to unblock sooner I think the first option is better and the reviewer in the V8 CL seems to agree - you could chime in in the CL too about what you think is better, thanks |
|
https://chromium-review.googlesource.com/c/v8/v8/+/3575468 has landed, I think we can fix the TODOs here once our copy of V8 has that fix. |
|
Oops, sorry, we cannot address the TODOs here without another patch in V8, so either way this needs to go first to avoid breaking V8's CI. I will do some test to see if putting the snapshot request behind an interrupt affects the precision of the snapshot. |
|
I just remembered about this PR. So from what I can remember, the status is:
I think this one can be closed now because of 1. I will open a separate PR to make the extra leeway less conservative. |
limit, as that is what V8 uses to determine whether it
should crash - previously we use the used size for the
calculation and that was too conservative and did not
prevent the crashes effectively enough.
need to make sure that the heap limit is raised first
before the snapshot can be taken.
Refs: #42115
Fixes: nodejs/node-v8#218