-
Notifications
You must be signed in to change notification settings - Fork 66
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
Heap Allocation changes causing DCHECKs in heap snapshotting #282
Comments
/cc @joyeecheung |
From the stack trace it seems somehow the no-allocation invariant is enforced during snapshot serialization (I am pretty sure the original intent was to only apply it to generation? cc @jdapena). If that’s the case it seems strange that this passes the Node.js integration tests in V8 CI and Node.js’s own CI and only get caught in Electron’s test suite. |
We don't have debug CI for canary builds. |
Here's a test build: https://ci.nodejs.org/job/node-test-commit-arm-debug/12857/ |
^ Reproduced |
I will take a look. The idea was also covering the JSON serialization stage, as we do not want the heap to be modified for generating a heap snapshot, not even on serialization step. I will need to remove the check for serialization stage, and add it explicitely on tests that we know are not going to creates themselves heap buffers in serialization stage. |
Right for Node.js’s use case the serialization needs to allow allocation because there is an API that allows users to consume the heap snapshot in a readable stream in JavaScript. It doesn’t seem very useful to keep that restriction beyond heap snapshot generation. |
I am preparing a fix in V8 side: https://chromium-review.googlesource.com/c/v8/v8/+/5531355 |
In our latest V8 roll via Chromium update, we're seeing a lot of crashes in heap snapshot tests - we've tracked these to https://bugs.chromium.org/p/v8/issues/detail?id=14617 and the associated CLs therein.
Failing tests we observed:
The stacktraces for each differ subtly, but one example is below
Sample Stacktrace
We've reverted these in Electron temporarily (see electron/electron#42125) but they should likely hit you all in canary soon! Happy to help with a PR once there's a path forward.
The text was updated successfully, but these errors were encountered: