Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1491203002:
Updated the check for unmodfied objects to handle Smi Objects. (Closed)
DescriptionUpdated the check for unmodfied objects to handle Smi Objects.
The new minorGC pass collects all unmodified objects that are not marked
active by blink. The earlier implementation assumed all new space nodes
to be Heap objects. Updated this code to handle Smi objects as well.
BUG=553287
LOG=Y
Committed: https://crrev.com/8e771d9b773538d3d87695d2dbbed33c9ab5cce6
Cr-Commit-Position: refs/heads/master@{#32704}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed review comments. Does not mark any objects that are not created via C++ API as unmodified. #Messages
Total messages: 18 (8 generated)
mythria@chromium.org changed reviewers: + jochen@chromium.org
Hi Jochen, I fixed this bug https://code.google.com/p/chromium/issues/detail?id=553287 in minor GC. Could please review my changes. Thanks, Mythri https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1496 src/heap/heap.cc:1496: if (object->IsSmi()) return true; I thought we can collect all Smi objects. If you think it is more expensive to collect them and recreate them when necessary, I will just return false. In that case they will not be collected by minor GC. https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1496 src/heap/heap.cc:1496: if (object->IsSmi()) return true; I am a little surprised that we have Smi objects that are marked weak in new space nodes. Is it expected? This is the code which generates a Weak Smi object: Promise.reject(42); from the test inspector/console/console-uncaught-promise.html in LayoutTests https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1497 src/heap/heap.cc:1497: DCHECK(object->IsHeapObject()); I think we could remove this DCHECK right? or is it safer to have it?
https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1496 src/heap/heap.cc:1496: if (object->IsSmi()) return true; On 2015/12/02 at 09:45:49, mythria wrote: > I am a little surprised that we have Smi objects that are marked weak in new space nodes. Is it expected? This is the code which generates a Weak Smi object: > > Promise.reject(42); from the test inspector/console/console-uncaught-promise.html in LayoutTests actually, I think we should return false for all objects that weren't created via the C++ API. While it's possible to recreate e.g. 42, blink has no way of knowing that it should recreate 42 https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1497 src/heap/heap.cc:1497: DCHECK(object->IsHeapObject()); On 2015/12/02 at 09:45:49, mythria wrote: > I think we could remove this DCHECK right? or is it safer to have it? you can remove it
Could you please review my changes Thanks, Mythri https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1496 src/heap/heap.cc:1496: if (object->IsSmi()) return true; On 2015/12/02 09:48:28, jochen wrote: > On 2015/12/02 at 09:45:49, mythria wrote: > > I am a little surprised that we have Smi objects that are marked weak in new > space nodes. Is it expected? This is the code which generates a Weak Smi object: > > > > Promise.reject(42); from the test > inspector/console/console-uncaught-promise.html in LayoutTests > > actually, I think we should return false for all objects that weren't created > via the C++ API. > > While it's possible to recreate e.g. 42, blink has no way of knowing that it > should recreate 42 Done. https://codereview.chromium.org/1491203002/diff/1/src/heap/heap.cc#newcode1497 src/heap/heap.cc:1497: DCHECK(object->IsHeapObject()); On 2015/12/02 09:48:28, jochen wrote: > On 2015/12/02 at 09:45:49, mythria wrote: > > I think we could remove this DCHECK right? or is it safer to have it? > > you can remove it Done.
lgtm
jochen@chromium.org changed reviewers: + hpayer@chromium.org
+hpayer - ok to land or is the heap still melting down?
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491203002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/8711)
Description was changed from ========== Updated the check for unmodfied objects to handle Smi Objects. The new minorGC pass collects all unmodified objects that are not marked active by blink. The earlier implementation assumed all new space nodes to be Heap objects. Updated this code to handle Smi objects as well. BUG=553287 ========== to ========== Updated the check for unmodfied objects to handle Smi Objects. The new minorGC pass collects all unmodified objects that are not marked active by blink. The earlier implementation assumed all new space nodes to be Heap objects. Updated this code to handle Smi objects as well. BUG=553287 LOG=Y ==========
The CQ bit was checked by mythria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491203002/20001
Message was sent while issue was closed.
Description was changed from ========== Updated the check for unmodfied objects to handle Smi Objects. The new minorGC pass collects all unmodified objects that are not marked active by blink. The earlier implementation assumed all new space nodes to be Heap objects. Updated this code to handle Smi objects as well. BUG=553287 LOG=Y ========== to ========== Updated the check for unmodfied objects to handle Smi Objects. The new minorGC pass collects all unmodified objects that are not marked active by blink. The earlier implementation assumed all new space nodes to be Heap objects. Updated this code to handle Smi objects as well. BUG=553287 LOG=Y ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Updated the check for unmodfied objects to handle Smi Objects. The new minorGC pass collects all unmodified objects that are not marked active by blink. The earlier implementation assumed all new space nodes to be Heap objects. Updated this code to handle Smi objects as well. BUG=553287 LOG=Y ========== to ========== Updated the check for unmodfied objects to handle Smi Objects. The new minorGC pass collects all unmodified objects that are not marked active by blink. The earlier implementation assumed all new space nodes to be Heap objects. Updated this code to handle Smi objects as well. BUG=553287 LOG=Y Committed: https://crrev.com/8e771d9b773538d3d87695d2dbbed33c9ab5cce6 Cr-Commit-Position: refs/heads/master@{#32704} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8e771d9b773538d3d87695d2dbbed33c9ab5cce6 Cr-Commit-Position: refs/heads/master@{#32704} |
