Scavenger does not collect unreachable non-API objects with weak handles |
||||||||||
Issue descriptionContext: https://github.com/GoogleCloudPlatform/cloud-profiler-nodejs/issues/199#issuecomment-389331700 ofrobots@ and I found a bug in is_active logic of global handles. The problem is that we mark a weak handle as active if it is not an JSObject::IsUnmodifiedApiObject: void GlobalHandles::IdentifyWeakUnmodifiedObjects( WeakSlotCallback is_unmodified) { for (Node* node : new_space_nodes_) { if (node->IsWeak() && !is_unmodified(node->location())) { node->set_active(true); } } } For any non-API object the predicate IsUnmodifiedApiObject is false, which causes the node to be marked as active and prevents its garbage collection even if the object is unreachable. For example, a heap number pointed to by a weak handle cannot be collected in Scavenger (this happens in the NodeJS repro). Formally: !IsUnmodifiedApiObject() is not the same as IsModifiedApiObject().
,
Jun 4 2018
Was just thinking this through again. We still have the flag to set whether something is active or not? You said that we cannot collect weak handles to regular objects as that would otherwise also collect the leafs that point into V8. Now, those leafs should be marked as active from the blink side before doing a Scavenger. It should thus be fine to collect all other weak handles that are unmodified (and not API).
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0c5096130bd991e6144c084b85a20927bb47010e commit 0c5096130bd991e6144c084b85a20927bb47010e Author: Ulan Degenbaev <ulan@chromium.org> Date: Mon Jun 04 18:33:33 2018 Revert "Global handles: Remove independent handle infrastructure" This reverts 0944553ee8498bda5b9897871e29c70e9c5ae00e. This is a short-term fix for NodeJS regression caused by Scavenger not collecting weak handles that are marked as independent. Bug: chromium:847863, chromium:780749 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: Ibd5224893c64baef1aaaecd18af94f29e2e74487 Reviewed-on: https://chromium-review.googlesource.com/1082439 Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Reviewed-by: Hannes Payer <hpayer@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Cr-Commit-Position: refs/heads/master@{#53498} [modify] https://crrev.com/0c5096130bd991e6144c084b85a20927bb47010e/include/v8.h [modify] https://crrev.com/0c5096130bd991e6144c084b85a20927bb47010e/src/global-handles.cc [modify] https://crrev.com/0c5096130bd991e6144c084b85a20927bb47010e/src/global-handles.h
,
Jun 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/aaa700bda4077d0f01db778052db598c097d5273 commit aaa700bda4077d0f01db778052db598c097d5273 Author: Ulan Degenbaev <ulan@chromium.org> Date: Mon Jun 04 19:13:20 2018 Revert "[heap] Remove independent handles" This reverts 667555c6b8dadcfe4e592fe06bb6846468b8141e. This is a short-term fix for NodeJS regression caused by Scavenger not collecting weak handles that are marked as independent. Bug: chromium:847863, chromium:780749 Change-Id: Ia1c02e042d0e593c6f5badb82c4ef20b923d3806 Reviewed-on: https://chromium-review.googlesource.com/1082442 Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Hannes Payer <hpayer@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#53502} [modify] https://crrev.com/aaa700bda4077d0f01db778052db598c097d5273/src/global-handles.cc [modify] https://crrev.com/aaa700bda4077d0f01db778052db598c097d5273/src/profiler/sampling-heap-profiler.cc [modify] https://crrev.com/aaa700bda4077d0f01db778052db598c097d5273/test/cctest/test-api.cc
,
Jun 5 2018
,
Jun 5 2018
I have verified that the reverts fix the problem in my testing. I would like this merged back to V8 6.7 and 6.8 so that we can pick it up for Node.js 10.x.
,
Jun 5 2018
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 5 2018
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 5 2018
Pls apply appropriate OSs label. Thank you.
,
Jun 5 2018
govind@, the patches do not affect Chrome as they change a function that is not used by Chrome. We need these patches for other embedders of V8.
,
Jun 5 2018
hablich@ for Review. Pls note for Desktop we already cut M67 stable RC for release tomorrow, so it will miss this merge if approved.
,
Jun 6 2018
We should merge this back, the risk is low and the local impact is high: - No risk of affecting website visitors - Fixes a memory regression in DevTools - Fixes a regression in Node.js
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/ee8854c8cb0717b7cb63987e6ae9c554c829ebd0 commit ee8854c8cb0717b7cb63987e6ae9c554c829ebd0 Author: Ulan Degenbaev <ulan@chromium.org> Date: Thu Jun 07 13:23:05 2018 Merged: Squashed multiple commits. Merged: Revert "Global handles: Remove independent handle infrastructure" Revision: 0c5096130bd991e6144c084b85a20927bb47010e Merged: Revert "[heap] Remove independent handles" Revision: aaa700bda4077d0f01db778052db598c097d5273 BUG= chromium:780749 ,chromium:847863 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=hablich@chromium.org Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: Ib0204cffc37c7a93dcbb2d1cf7b19e31346b5a15 Reviewed-on: https://chromium-review.googlesource.com/1090833 Reviewed-by: Michael Hablich <hablich@chromium.org> Cr-Commit-Position: refs/branch-heads/6.8@{#19} Cr-Branched-From: 44d7d7d6b1041b57644400a00cb3fee35f6c51b2-refs/heads/6.8.275@{#1} Cr-Branched-From: 5754f66f75136dc17b4c63fec84f31dfdb89186e-refs/heads/master@{#53286} [modify] https://crrev.com/ee8854c8cb0717b7cb63987e6ae9c554c829ebd0/include/v8.h [modify] https://crrev.com/ee8854c8cb0717b7cb63987e6ae9c554c829ebd0/src/global-handles.cc [modify] https://crrev.com/ee8854c8cb0717b7cb63987e6ae9c554c829ebd0/src/global-handles.h [modify] https://crrev.com/ee8854c8cb0717b7cb63987e6ae9c554c829ebd0/src/profiler/sampling-heap-profiler.cc [modify] https://crrev.com/ee8854c8cb0717b7cb63987e6ae9c554c829ebd0/test/cctest/test-api.cc
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d89f1638856564ca66701ce64ffaf474522cd206 commit d89f1638856564ca66701ce64ffaf474522cd206 Author: Ulan Degenbaev <ulan@chromium.org> Date: Thu Jun 07 13:30:57 2018 Merged: Squashed multiple commits. Merged: Revert "Global handles: Remove independent handle infrastructure" Revision: 0c5096130bd991e6144c084b85a20927bb47010e Merged: Revert "[heap] Remove independent handles" Revision: aaa700bda4077d0f01db778052db598c097d5273 BUG= chromium:780749 ,chromium:847863 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=hablich@chromium.org Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I0af773f9c130b85a58dd22204a2933a48a4a7b35 Reviewed-on: https://chromium-review.googlesource.com/1090843 Reviewed-by: Michael Hablich <hablich@chromium.org> Cr-Commit-Position: refs/branch-heads/6.7@{#86} Cr-Branched-From: 8457e810efd34381448d51d93f50079cf1f6a812-refs/heads/6.7.288@{#2} Cr-Branched-From: e921be5c4f2c6407936bde750992dedbf47c1016-refs/heads/master@{#52547} [modify] https://crrev.com/d89f1638856564ca66701ce64ffaf474522cd206/include/v8.h [modify] https://crrev.com/d89f1638856564ca66701ce64ffaf474522cd206/src/global-handles.cc [modify] https://crrev.com/d89f1638856564ca66701ce64ffaf474522cd206/src/global-handles.h [modify] https://crrev.com/d89f1638856564ca66701ce64ffaf474522cd206/src/profiler/sampling-heap-profiler.cc [modify] https://crrev.com/d89f1638856564ca66701ce64ffaf474522cd206/test/cctest/test-api.cc
,
Jun 11 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 15 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 18 2018
,
Jun 27 2018
This was an issue in Node 10 only, so no more backports are needed. Add 'nodejs-backport-rejected' label. |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by u...@chromium.org
, Jun 1 2018