Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
Fix segfault during GC #5900 (4.x)#7303
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: nodejs#5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871
This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: nodejs#5900
targos commented Jun 15, 2016
The patch LGTM. |
bnoordhuis commented Jun 15, 2016
LGTM. Do the V8 and node.js test suite pass with this change? |
ofrobots commented Jun 15, 2016
indutny commented Jun 15, 2016
@ofrobots do we have understand of what particular call site is causing segfault? Rubber-stamp LGTM |
ofrobots commented Jun 15, 2016
@indutny The crash happens when the StoreBuffer is iterating pointers to new space: |
ofrobots commented Jun 15, 2016
And there was a secondary bug in |
ofrobots commented Jun 15, 2016
It seems like it is not possible to run the V8 tests in CI on the v4.x branch? (/cc @mhdawson). See https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/135/. I will run them manually and report back. |
ofrobots commented Jun 16, 2016
V8 tests pass for me when run manually on a mac. |
ed3d372 to f14d9cfCompareMylesBorins commented Jun 30, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
running v8 tests in CI https://ci.nodejs.org/job/node-test-commit-v8-linux/171/ Will land if green |
MylesBorins commented Jul 1, 2016
CI is green LGTM |
This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: #5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871 PR-URL: #7303 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]>
This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: #5900 PR-URL: #7303 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]>
MylesBorins commented Jul 1, 2016
landed in 5ba807a...e319d76 |
This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: #5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871 PR-URL: #7303 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]>
This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: #5900 PR-URL: #7303 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Original commit message: deps: backport e7cc609 from upstream V8 This is part 1/2 of the fixes from v8:4871. This fixes a segfault in verify-heap. Original commit message: [crankshaft] Write fillers for folded old space allocations during verify-heap If we don't write fillers, we crash during PagedSpace verification when we try to iterate over dead memory (unused folded allocation slots). BUG=v8:4871,chromium:580959 LOG=N Review URL: https://codereview.chromium.org/1837163002 Cr-Commit-Position: refs/heads/master@{#35097} Fixes: nodejs/node#5900 V8-Bug: https://bugs.chromium.org/p/v8/issues/detail?id=4871 PR-URL: nodejs/node#7303 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Original commit message: deps: fix segfault during gc This is part 2/2 of the fixes needed for v8:4871. This fix never landed upstream because the bug is not present in active V8 version. The patch is available from the upstream v8 bug however. The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3). This patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications. Fixes: nodejs/node#5900 PR-URL: nodejs/node#7303 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Checklist
make -j4 test(UNIX) orvcbuild test nosign(Windows) passesAffected core subsystem(s)
deps: v8
Description of change
These two patches comprise a fix for #5900 – a segfault during GC that can happen under rare circumstances. The interesting part is that the one of the fixes never landed upstream in V8 because the bug doesn't exist in any active V8 branches.
The segfault occurs at the intersection of the following three conditions that are dependent on the allocation pattern of an application: A pretenured (1) allocation site has to be optimized into a merged allocation by the allocation folding optimization (2) and there needs to be overflow of the store buffer (3).
This second patch disables the allocation folding optimization for pretenured allocations. This may have some, hopefully negligible, performance impact on real world applications.
This also needs to be fixed in
v5.x; but I'm not sure how much runway is left on that branch and whether it would give us sufficient feedback. Regardless, I think an independent determination can be made whether this is worth fixing onv4.xin the first place (given than the scenario is rare).R=@nodejs/lts
/cc @nodejs/v8