Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078403002:
[heap] Filter out stale left-trimmed handles (Closed)
|
Created:
4 years, 6 months ago by Michael Lippautz Modified:
4 years, 6 months ago CC:
ulan, v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[heap] Filter out stale left-trimmed handles
BUG=chromium:620553
LOG=N
R=jochen@chromium.org
Committed: https://crrev.com/d800a65967b115c6e1aa6c3ba08861a304383088
Cr-Commit-Position: refs/heads/master@{#37108}
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressed comment #Patch Set 3 : Remove check that ensure that only a single handle points to left-trimmed array #
Messages
Total messages: 27 (14 generated)
Description was changed from ========== [heap] Filter out stale lef-trimmed handles BUG=chromium:620553 LOG=N ========== to ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N ========== to ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.orgcbruni@chromium.org ==========
mlippautz@chromium.org changed reviewers: + cbruni@chromium.org, jochen@chromium.org
Description was changed from ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.orgcbruni@chromium.org ========== to ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org,cbruni@chromium.org ==========
jochen: ptal. I cannot think of any nice way to better handle the stale Handle (;p) camillo: Please check the test. I think this matches exactly what you told me wrt. to getter behavior.
can you explain why we can't avoid them? https://codereview.chromium.org/2078403002/diff/20001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2078403002/diff/20001/src/heap/mark-compact.c... src/heap/mark-compact.cc:1423: // We cannot avoid stale handles to left-trimmed objects and only make sure nit: s/and only/, but can only/
Elements accessors in elements.cc go through some obstacles to make sure no GC happens where they have a stale handle in a scope. However, in general it is not possible to avoid those scenarios because calling out into JS makes anything possible. The regression example uses shift() [calling the left-trim] and a combination of concat() and __defineGetter__ to create a Handle [for the receiver] and a recursion [calling the getter again]. https://codereview.chromium.org/2078403002/diff/20001/src/heap/mark-compact.cc File src/heap/mark-compact.cc (right): https://codereview.chromium.org/2078403002/diff/20001/src/heap/mark-compact.c... src/heap/mark-compact.cc:1423: // We cannot avoid stale handles to left-trimmed objects and only make sure On 2016/06/20 09:53:27, jochen wrote: > nit: s/and only/, but can only/ Done.
I wonder whether there's opportunity to simplify elements.cc anyways, lgtm
On 2016/06/20 12:45:39, jochen wrote: > I wonder whether there's opportunity to simplify elements.cc Ack.
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078403002/40001
Description was changed from ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org,cbruni@chromium.org ========== to ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org ==========
The CQ bit was unchecked by mlippautz@chromium.org
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078403002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2078403002/#ps60001 (title: "Remove check that ensure that only a single handle points to left-trimmed array")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078403002/60001
Message was sent while issue was closed.
Description was changed from ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org ========== to ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org ========== to ========== [heap] Filter out stale left-trimmed handles BUG=chromium:620553 LOG=N R=jochen@chromium.org Committed: https://crrev.com/d800a65967b115c6e1aa6c3ba08861a304383088 Cr-Commit-Position: refs/heads/master@{#37108} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d800a65967b115c6e1aa6c3ba08861a304383088 Cr-Commit-Position: refs/heads/master@{#37108}
Message was sent while issue was closed.
On 2016/06/20 14:32:32, commit-bot: I haz the power wrote: > Patchset 3 (id:??) landed as > https://crrev.com/d800a65967b115c6e1aa6c3ba08861a304383088 > Cr-Commit-Position: refs/heads/master@{#37108} This is very unfortunate, each object has to point to a valid object. A filler is not a valid object. This is one of the main invariants of our heap. I would be in favor of not doing this fix, but changing it on the use level. This bug demonstrates again why moving the object header is problematic.
Message was sent while issue was closed.
On 2016/06/20 22:37:25, Hannes Payer (slow) wrote: > On 2016/06/20 14:32:32, commit-bot: I haz the power wrote: > > Patchset 3 (id:??) landed as > > https://crrev.com/d800a65967b115c6e1aa6c3ba08861a304383088 > > Cr-Commit-Position: refs/heads/master@{#37108} > > This is very unfortunate, each object has to point to a valid object. A filler > is not a valid object. This is one of the main invariants of our heap. I would > be in favor of not doing this fix, but changing it on the use level. This bug > demonstrates again why moving the object header is problematic. Ack. Let's talk offline. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
