-
Notifications
You must be signed in to change notification settings - Fork 30k
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 crash during MarkCompact GC #7259
Comments
I agree, it seems to stem from V8 using It looks fairly straightforward to fix but I don't have time to write up a patch. Does someone from @nodejs/v8 want to take this on? |
@papandreou You should file a bug over at https://bugs.chromium.org/p/v8/issues/list too. |
@bnoordhuis Thanks a lot for taking a look. I filed a sister issue here: https://bugs.chromium.org/p/v8/issues/detail?id=5094 |
Fixed for V8 5.6, according to @mlippautz on https://bugs.chromium.org/p/v8/issues/detail?id=5094#c9 I'll rerun the experiment once a node.js built against a fixed V8 is available. |
@papandreou the latest nightly builds have V8 5.6: https://nodejs.org/download/nightly/ |
Saw a similar error in node v7.10.1 with a long-running script that had been running just fine for months:
|
@dandv Node 7.x release line is no longer supported, you should move to one of the supported release lines. |
What steps will reproduce the problem?
Check out https://github.com/gustavnikolaj/node6-gc-overflow and run https://github.com/gustavnikolaj/node6-gc-overflow/blob/4766e784cadbf795f7be4665012a68de1d20ab13/index.js
It will fail reliably with node 6.2.1, but it works fine with 5.11.1 (v8 version 4.6.85.31).
What is the expected output?
A single line of "DONE!" to stdout.
Like below:
What do you see instead?
Background
We use assetgraph-builder to build production versions of our web application. It's a tool that will take a web application and transform the code in various ways.
It will find style sheets and concatenate them, pick up on images that is referenced and optimize them etc. Because it loads all the assets into memory we have had to use the
--max-old-space-size
flag to avoid running out of memory.When we attempted to upgrade one of our larger web applications to node.js v6 we ran into the error described above. From what we have gathered, it happens when the garbage collector is attempting to clear more than ~2.2GB of memory using the MarkCompact strategy.
node/deps/v8/src/heap/spaces.h
Lines 1510 to 1517 in c7b0d06
Our theory is that the signed 32 bit signed ints are overflowed and thus cause the check to fail. The only difference in the ShrinkSpace function between v8 4.6 and v8 5.0 is that the check has been promoted from a debug-check to a run time check, so we have most likely been exposed to this error longer than we were aware. The commit message suggests that the V8 team is already trying to root cause the problem: v8/v8@1682935
We have attempted to patch deps/v8 ourselves by naively changing the int types to
size_t
but without luck, so we suspect that it's a bug in the actual calculation algorithm.Reproducing the bug with V8
We've also tried and failed to reduce the code snippet to something that will also trigger the bug in
v8_shell
, which means thatBuffer
cannot be used. None of our attempts have been able to trigger a MarkCompact GC, though, but that might be due to a lack of understanding of the heuristics employed by V8. This is the reason why we're reporting the bug here initially.The text was updated successfully, but these errors were encountered: