Remove SmartMove, bringing Array methods further into spec compliance

Remove SmartMove, bringing Array methods further into spec compliance

This is one step towards a single codepath for each method in array.js.

This patch is based on rafaelw's https://codereview.chromium.org/349073002

BUG=v8:2615
LOG=Y
R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24647

[adamk, 2014-08-08 19:04:31]

I believe this patch addresses Toon's concerns from Rafael's original path, save
the note about naming (which I haven't fixed only because I'm not sure what to
do with SmartSlice/SimpleSlice).

[adamk, 2014-08-12 20:37:29]

Review ping?

[Toon Verwaest, 2014-08-13 16:48:50]

So overall this LGTM. After seeing danno@ struggle with keeping performance for
dictionary-mode objects up, however, it seems like we should pretty soon have an
optimization layer for that kind of object. Are you planning on working on that
as well? Can we wait with landing this until we have that?

[adamk, 2014-08-13 20:28:11]

On 2014/08/13 at 16:48:50, verwaest wrote:
> So overall this LGTM. After seeing danno@ struggle with keeping performance
for dictionary-mode objects up, however, it seems like we should pretty soon
have an optimization layer for that kind of object. Are you planning on working
on that as well? Can we wait with landing this until we have that?

I was planning to do the dictionary-mode handling in a (relatively soon)
followup, but I think it's reasonable to just hold back on landing this until we
have that done.

[adamk, 2014-10-06 20:40:37]

On 2014/08/13 at 20:28:11, adamk wrote:
> On 2014/08/13 at 16:48:50, verwaest wrote:
> > So overall this LGTM. After seeing danno@ struggle with keeping performance
for dictionary-mode objects up, however, it seems like we should pretty soon
have an optimization layer for that kind of object. Are you planning on working
on that as well? Can we wait with landing this until we have that?
> 
> I was planning to do the dictionary-mode handling in a (relatively soon)
followup, but I think it's reasonable to just hold back on landing this until we
have that done.

Okay, finally got around to looking at this again. Having spent a bit of time
trying to write a "sparse" version of unshift, I'm not sure it's doable in a
spec-compliant way without a good deal of extra work. The big problem is that
getters and setters invoked as part of the unshift algorithm can themselves
mutate the array, potentially adding new indices. See
https://code.google.com/p/v8/issues/detail?id=3612 for exactly this issue with
the existing SparseReverse() function.

I'm inclined, given the above, to land this without adding any SparseMove()
function, as getting that correct seems like it might be more trouble than it's
worth. Thoughts?

[adamk, 2014-10-06 20:41:54]

adamk@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[adamk, 2014-10-06 20:42:11]

+mstarzinger since verwaest is currently out.

[Toon Verwaest, 2014-10-15 08:35:50]

lgtm

[adamk, 2014-10-15 22:07:17]

Rebased against current bleeding_edge. Note that I also had to update
regress-crbug-412319.js to do an operation that's valid on a non-extensible
object (pop() instead of push()). I verified that the regression test is still
testing the builtin inlining code in hydrogen.cc.

[adamk, 2014-10-15 23:37:05]

Committed patchset #3 (id:40001) manually as 24647 (presubmit successful).

[adamk, 2014-10-15 23:53:51]

Reverted in https://code.google.com/p/v8/source/detail?r=24648 :(