Make sure that we grow the slots buffer when we are in evacuation scope.

Make sure that we grow the slots buffer when we are in evacuation scope.

BUG=

[ulan, 2014-03-05 13:11:25]

lgtm

[Hannes Payer (out of office), 2014-03-05 15:20:54]

Please take another look. As suggested, I landed the actual fix in a separate
cl. Moreover, I am moving quite some code around.

[ulan, 2014-03-05 15:40:18]

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h
File src/mark-compact.h (right):

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h#newc...
src/mark-compact.h:384: static const int kNumberOfElements = 1020;
Why is this change to 1020 necessary?

[Hannes Payer (out of office), 2014-03-05 20:39:28]

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h
File src/mark-compact.h (right):

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h#newc...
src/mark-compact.h:384: static const int kNumberOfElements = 1020;
On 2014/03/05 15:40:18, ulan wrote:
> Why is this change to 1020 necessary?
The slots buffer should fit on a page. After adding one word (heap pointer), it
would overflow the page.

[Michael Starzinger, 2014-03-06 10:47:25]

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact-inl.h
File src/mark-compact-inl.h (right):

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact-inl.h#...
src/mark-compact-inl.h:108: mode == FAIL_ON_OVERFLOW);
This invariant can be expressed in one expression I think.

ASSERT(heap->mark_compact_collector()->IsInsideEvacuationScope() ==
       mode != IGNORE_OVERFLOW);

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact-inl.h#...
src/mark-compact-inl.h:132: mode == FAIL_ON_OVERFLOW);
Likewise.

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.cc#new...
src/mark-compact.cc:4218: EvacuationScope no_evacuation(this);
nit: The naming of this scope (the local variable, not the class) is weird,
shouldn't it be called "in_evacuation" instead, because it indicates we are
evacuating now?

Also, can we move it to the top of the function, to right after the "gc_scope"?

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h
File src/mark-compact.h (right):

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h#newc...
src/mark-compact.h:327: 
nit: These two should be made protected IMHO.

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h#newc...
src/mark-compact.h:346: inline void UpdateSlotsRecordedIn(bool
code_slots_filtering_required) {
nit: The naming is a little bit weird now, because it no longer takes an
argument to which the "in" applies. Can we rename the method to
"UpdateRecordedSlots" instead?

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h#newc...
src/mark-compact.h:743: ASSERT(evacuation_scope_ <= 1);
These two ASSERTS combined basically enforce ...

ASSERT(evacuation_scope_ == 1);  // in IncrementEvacuationScope
ASSERT(evacuation_scope_ == 0);  // in DecrementEvacuationScope

Can we change it to this instead? That would be much clearer.

https://codereview.chromium.org/187683002/diff/130001/src/mark-compact.h#newc...
src/mark-compact.h:1048: class EvacuationScope V8_FINAL {
High-level idea: This scope pretty much coincides with "state_ == SWEEP_SPACES",
would it make sense to use the state variable instead, or do you think a
separate scope brings additional advantages?

[Hannes Payer (out of office), 2014-04-30 07:50:22]

This cl became obsolete with v8 revision r21042. I am closing this issue.