[elements] Enable left-trimming again

[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc which happened in the mean time.

drive-by-fix: pass isolate as argument to more functions in elements.cc.

BUG=v8:4606
LOG=y

Committed: https://crrev.com/31779283ed16d630d1e1d4f9c28592e5cb90e635
Cr-Commit-Position: refs/heads/master@{#33023}

[Camillo Bruni, 2015-12-21 16:17:57]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-21 16:18:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1543563002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1543563002/20001

[commit-bot: I haz the power, 2015-12-21 16:36:48]

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

[commit-bot: I haz the power, 2015-12-21 16:36:48]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-12-21 16:39:09]

cbruni@chromium.org changed reviewers:
+ ishell@chromium.org

[Camillo Bruni, 2015-12-21 16:39:10]

PTAL
I introduced left-trim support for elements.cc again. I did some additional
cleanup work to maintain the existing refactorings in place.

[Camillo Bruni, 2015-12-21 16:40:32]

https://codereview.chromium.org/1543563002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1543563002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:3118: printf("%d\n", elements_to_trim);
ok this has to go...

[Igor Sheludko, 2015-12-22 10:02:36]

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc#newcode...
src/elements.cc:1581: dst_elms = Handle<FixedArray>(
Since
1) the left trimming is not "safe" in a sense that we would crash anyway if we
ever create a handle to untrimmed object before entering elements accessors and
then try to use it after left trimming,
2) the approach to return an updated handle from MoveElements does not solve (1)
in general,
maybe it is better just to update the existing backing_store handle and keep the
signature of MoveElements unchanged.

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc#newcode...
src/elements.cc:1585: hole_end -= src_index;
Shouldn't we update hole_start here as well?

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc#newcode...
src/elements.cc:1714: dst_elms = Handle<FixedDoubleArray>(
Ditto...

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc#newcode...
src/elements.cc:1719: hole_end -= src_index;
Ditto...

[Camillo Bruni, 2015-12-22 12:52:14]

Description was changed from

==========
[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc.


BUG=
==========

to

==========
[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc which happened in the mean time.

drive-by-fix: pass isolate as argument to more functions in elements.cc.


BUG=4606
LOG=y
==========

[Camillo Bruni, 2015-12-22 12:52:26]

Description was changed from

==========
[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc which happened in the mean time.

drive-by-fix: pass isolate as argument to more functions in elements.cc.


BUG=4606
LOG=y
==========

to

==========
[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc which happened in the mean time.

drive-by-fix: pass isolate as argument to more functions in elements.cc.


BUG=v8:4606
LOG=y
==========

[Camillo Bruni, 2015-12-22 14:09:19]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 14:09:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1543563002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1543563002/60001

[Camillo Bruni, 2015-12-22 14:21:01]

PTAL again.
Updating handles in-place now, gc stress-tests still pending.

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1543563002/diff/40001/src/elements.cc#newcode...
src/elements.cc:1585: hole_end -= src_index;
On 2015/12/22 at 10:02:36, Igor Sheludko wrote:
> Shouldn't we update hole_start here as well?

I added a DCHECK, the hole_start has to lie within length-removed elements (aka.
in the worst case right at the end of storage if we only trim).

[commit-bot: I haz the power, 2015-12-22 14:27:57]

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

[commit-bot: I haz the power, 2015-12-22 14:27:57]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2015-12-22 17:15:01]

https://codereview.chromium.org/1543563002/diff/100001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1543563002/diff/100001/src/elements.cc#newcod...
src/elements.cc:1582: dst_elms = Handle<FixedArray>(
Sorry for confusion. I actually meant to do this:
  *dst_elms.location() = FixedArray::cast(heap->Left...);

This way you don't even need to touch the backing_store handle because they
share location. And all the other copies would be updated automatically and you
don't have to pass backing_store by reference.

https://codereview.chromium.org/1543563002/diff/100001/src/elements.cc#newcod...
src/elements.cc:1585: backing_store = dst_elms;
Not needed once the above comment is addressed.

https://codereview.chromium.org/1543563002/diff/100001/src/elements.cc#newcod...
src/elements.cc:1718: dst_elms = Handle<FixedDoubleArray>(
Ditto.

https://codereview.chromium.org/1543563002/diff/100001/src/elements.cc#newcod...
src/elements.cc:1722: backing_store = dst_elms;
Ditto.

[Camillo Bruni, 2015-12-23 09:32:19]

PTAL again :)
I implementin in-place handler updating now for the left-trimmed case.

[Camillo Bruni, 2015-12-23 10:01:55]

PTAL :)

[Igor Sheludko, 2015-12-23 10:15:08]

LGTM! :)

[Camillo Bruni, 2015-12-23 10:54:14]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2015-12-23 10:54:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1543563002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1543563002/160001

[commit-bot: I haz the power, 2015-12-23 11:48:46]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-23 11:49:29]

Description was changed from

==========
[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc which happened in the mean time.

drive-by-fix: pass isolate as argument to more functions in elements.cc.


BUG=v8:4606
LOG=y
==========

to

==========
[elements] Enable left-trimming again

Essentially a revert of https://codereview.chromium.org/1346013005 but
preserving the refactorings in elements.cc which happened in the mean time.

drive-by-fix: pass isolate as argument to more functions in elements.cc.

BUG=v8:4606
LOG=y

Committed: https://crrev.com/31779283ed16d630d1e1d4f9c28592e5cb90e635
Cr-Commit-Position: refs/heads/master@{#33023}
==========

[commit-bot: I haz the power, 2015-12-23 11:49:31]

Patchset 9 (id:??) landed as
https://crrev.com/31779283ed16d630d1e1d4f9c28592e5cb90e635
Cr-Commit-Position: refs/heads/master@{#33023}