Check whether the marking deque overflowed in ProcessEphemeralMarking

Check whether the marking deque overflowed in ProcessEphemeralMarking

Otherwise, we might exit to early when we call ProcessEphemeralMarking
with an empty, overflown deque

BUG=none
R=hpayer@chromium.org,erik.corry@gmail.com
LOG=n

[Erik Corry, 2014-12-03 12:46:22]

https://codereview.chromium.org/777643002/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/777643002/diff/1/src/heap/mark-compact.cc#new...
src/heap/mark-compact.cc:2102: work_to_do = !marking_deque_.IsEmpty() ||
marking_deque_.overflowed();
It would be better to improve MarkCompactCollector::IsMarkingStackEmpty() to
include both these conditions and then use it here and in the DCHECK above. 
There are also about 5 places in incremental-marking.cc that check for an empty
stack, and at least some of them should use this check instead.

I see Hannes just moved that function, so maybe it should be changed in its new
location (incremental-marking.somthing)

[jochen (gone - plz use gerrit), 2014-12-03 12:48:57]

Hannes proposed to just have the marking deque grow unboundedly now that is no
longer in the new space, so it never overflows..

[Erik Corry, 2014-12-03 12:55:07]

That's what OIlpan does, but it's generally a much smaller heap.

[Erik Corry, 2014-12-03 13:09:12]

https://codereview.chromium.org/777643002/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/777643002/diff/1/src/heap/mark-compact.cc#new...
src/heap/mark-compact.cc:2094: DCHECK(marking_deque_.IsEmpty());
Thinking about it some more, we should just empty the deque here (including
overflows).  This method should not assume stuff about the situation coming in,
there's no reason for that.

And then there's no need to check for overflow in the loop, since you can't have
an empty deque and an overflow unless you somehow processed the deque, and if
you did that you broke things anyway because this function relies on being able
to see whether anything was pushed on the deque.

[Hannes Payer (out of office), 2014-12-03 13:18:24]

https://codereview.chromium.org/777643002/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/777643002/diff/1/src/heap/mark-compact.cc#new...
src/heap/mark-compact.cc:2102: work_to_do = !marking_deque_.IsEmpty() ||
marking_deque_.overflowed();
On 2014/12/03 12:46:21, Erik Corry wrote:
> It would be better to improve MarkCompactCollector::IsMarkingStackEmpty() to
> include both these conditions and then use it here and in the DCHECK above. 
> There are also about 5 places in incremental-marking.cc that check for an
empty
> stack, and at least some of them should use this check instead.
> 
> I see Hannes just moved that function, so maybe it should be changed in its
new
> location (incremental-marking.somthing)

Let's do that.

[jochen (gone - plz use gerrit), 2014-12-03 16:26:35]

i've uploaded a version that just reverts back to what we had before

how about we land that until the better fix is ready?

[Erik Corry, 2014-12-04 11:45:06]

LGTM

[jochen (gone - plz use gerrit), 2014-12-04 11:58:08]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-12-04 11:58:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777643002/20001

[commit-bot: I haz the power, 2014-12-04 11:58:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-04 11:58:38]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Erik Corry Chromium.org, 2014-12-04 12:17:00]

erikcorry@chromium.org changed reviewers:
+ erikcorry@chromium.org

[Erik Corry Chromium.org, 2014-12-04 12:17:01]

LGTM

[jochen (gone - plz use gerrit), 2014-12-04 13:43:24]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-12-04 13:44:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777643002/20001

[commit-bot: I haz the power, 2014-12-04 14:09:35]

Committed patchset #2 (id:20001)

[Hannes Payer (out of office), 2014-12-04 16:14:47]

lgtm