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 |
Tests in NodeJS with DCHECK enabled were failing because of two different problems: - One was that we were also disallowing heap allocations in the serialization stage. But NodeJS tests process the heap snapshot result in JS, so all those were broken. - But, also, the code that would retrieve from line ends would assume all the scripts populated line ends in the snapshot. But this was wrong. To fix it, this patch adds another storage of the line ends in the allocation tracker. This storage needs to keep weak references to the scripts so we do not leak the line ends data when scripts are disposed. This change keeps a weak reference to the scripts so, when they are freed, the line ends cache is also freed as it is not useful anymore. Node issue: nodejs/node-v8#282 Change-Id: I4314d707903175f0005893e9aa06d3ae52fc57f8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5531355 Commit-Queue: José Dapena Paz <jdapena@igalia.com> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Simon Zünd <szuend@chromium.org> Cr-Commit-Position: refs/heads/main@{#94418}
Original commit message:
[heap] Fix allocation tracker line end retrieval
Tests in NodeJS with DCHECK enabled were failing because of two
different problems:
- One was that we were also disallowing heap allocations in the
serialization stage. But NodeJS tests process the heap snapshot
result in JS, so all those were broken.
- But, also, the code that would retrieve from line ends would assume
all the scripts populated line ends in the snapshot. But this was
wrong. To fix it, this patch adds another storage of the line ends
in the allocation tracker. This storage needs to keep weak references
to the scripts so we do not leak the line ends data when scripts are
disposed.
This change keeps a weak reference to the scripts so, when they are
freed, the line ends cache is also freed as it is not useful anymore.
Node issue: nodejs/node-v8#282
Change-Id: I4314d707903175f0005893e9aa06d3ae52fc57f8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5531355
Commit-Queue: José Dapena Paz <jdapena@igalia.com>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#94418}
Refs: v8/v8@058870c
|
Should be fixed by https://chromium-review.googlesource.com/c/v8/v8/+/5531355, thanks @jdapena |
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: