-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
OOM does not report JS stacktrace or it's minimal #33532
Comments
The JS stack trace was removed in v8/v8@be5dd77 because collecting it can require memory when none is available. I think it should be safe to re-enable it again when That flag has an adverse effect on runtime performance, however. |
Oh it's an upstream change, good to know! Regardless, I should we should fix it from the node side. Stack trace is very valuable when debugging hard issues, and also 3 lines is not super helpful. Memory wise, it shouldn't be that big of a concern, especially in this case, since the process is going away. In my understanding this happens when node's heap limit is hit, but not the process is killed by the OOM killer, right? If it's the OOM killer, node doesn't even have a chance to print anything. So allocating like 32Kb of memory for the stack trace in this case shouldn't be that big of a deal. |
Unfortunately, it's not nearly as simple as that. For example, an out-of-memory condition can also happen when V8 cannot mmap the memory it needs. What's more, computing source positions means re-parsing the source code of the functions in the call stack, and that scales with the size of the functions, the number of variables, and so on. Expectation management: I'm not going to work on this (no time) but I'm leaving this open in case someone else wants to, or wants to take this up with V8. |
@bnoordhuis Sounds good, thanks for the clarification.
That all makes perfect sense. What I was saying is we should make the best effort to print the stack trace if possible.
Yeah that does sound very involved. Understandable. Also I wonder if it's possible to just expand the size of stacktrace (to be more than 512 bytes) in node 12, that could be beneficial and sounds easy enough. Downside is it's not future proof. |
@zen0wu - the only use case (IMO) where the current stack trace is usable in an OOM site is when the current code creates a large object. In all other cases, the OOM is a result of cumulative allocations in the past, for which the present stack trace has nothing to offer. Am I missing anything? |
I'd imagine the stack trace's usefulness would be correlated with how bad the memory leak is. The stack trace would be useful if the code is allocating a large object, yes, (100% useful) but also if it is allocating lots of tiny objects in a loop, (100%) and it'd also be useful probabilistically: if the offender is doing 75% of the allocations, there's a 75% chance the stack trace would be useful. |
I'm going to close this. There's been no movement in the last 1.5 years and it's an upstream issue so there isn't anything actionable. |
What steps will reproduce the bug?
Here's the code
How often does it reproduce? Is there a required condition?
node --max-old-space-size=128 test.js
What is the expected behavior?
When the process OOMs, it should print a useful JS stacktrace.
What do you see instead?
Node 12.16.2
Node 14.3.0
Additional information
As shown above, in Node 12, even though there's a stack trace from JS world, but it's very minimal because it only shows 3 frames, which is generally not too useful.
Since the process is going to abort, I don't think it's a big deal to extend the length of JS stack traces to provide more useful info. In fact I think the minimal stack trace is not really intentional.
With Node 14, it does not even show the stack trace anymore, which I believe should be a bug.
The text was updated successfully, but these errors were encountered: