[typedarrays] remove invalid optimization in NAMEConstructor()

[typedarrays] remove invalid optimization in NAMEConstructor()

Before, we were treating objects with the builtin ArrayValues iterator
method as array-like, where the iterator would iterate through to the
full length of the object.

This optimization was not sound, because it does not ensure that the
next method hasn't been modified. Even if it hasn't been modified,
it's entirely possible to be modified during iteration. Thus, this
optimization has been removed due to its observability.

BUG=v8:5699
R=littledan@chromium.org, cbruni@chromium.org

Committed: https://crrev.com/77df8c67d9609ada3b7d79e8e6d33f198bbad5a1
Cr-Commit-Position: refs/heads/master@{#41394}

[caitp, 2016-11-30 14:57:43]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 14:57:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[caitp, 2016-11-30 15:00:04]

PTAL. hopefully we can depend on ArrayIterator optimizations to make this
optimization less worthwhile than it used to be for typedarrays, anyways.

[commit-bot: I haz the power, 2016-11-30 15:32:32]

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

[commit-bot: I haz the power, 2016-11-30 15:32:33]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-11-30 16:02:28]

LGTM

What's actually the performance impact of this? just curious.

[caitp, 2016-11-30 16:15:51]

On 2016/11/30 16:02:28, Camillo Bruni wrote:
> LGTM
> 
> What's actually the performance impact of this? just curious.

Well, it's most likely a regression, but not clear how much of one. We don't
currently have benchmarks measuring this in v8 upstream, but I'd be surprised if
it didn't show up on the oortonline benchmark.

As mentioned on the bug, we could avoid removing the optimization entirely by
adding the following checks:

1. %ArrayIteratorPrototype%.next is still the original value
2. arrayLike object has fast elements
3. arrayLike object's length property is a data property
4. arrayLike object is not a JSProxy

But, it seemed like it probably wasn't worth it. Instead of a shortcut, this
needs a more sound optimization strategy

[Camillo Bruni, 2016-11-30 16:25:20]

On 2016/11/30 at 16:15:51, caitp wrote:
> On 2016/11/30 16:02:28, Camillo Bruni wrote:
> > LGTM
> > 
> > What's actually the performance impact of this? just curious.
> 
> Well, it's most likely a regression, but not clear how much of one. We don't
currently have benchmarks measuring this in v8 upstream, but I'd be surprised if
it didn't show up on the oortonline benchmark.
> 
> As mentioned on the bug, we could avoid removing the optimization entirely by
adding the following checks:
> 
> 1. %ArrayIteratorPrototype%.next is still the original value
> 2. arrayLike object has fast elements
> 3. arrayLike object's length property is a data property
> 4. arrayLike object is not a JSProxy
> 
> But, it seemed like it probably wasn't worth it. Instead of a shortcut, this
needs a more sound optimization strategy

Right, and probably that's easier in C++ than JavaScript.

[caitp, 2016-11-30 17:10:55]

On 2016/11/30 16:25:20, Camillo Bruni wrote:
> On 2016/11/30 at 16:15:51, caitp wrote:
> > On 2016/11/30 16:02:28, Camillo Bruni wrote:
> > > LGTM
> > > 
> > > What's actually the performance impact of this? just curious.
> > 
> > Well, it's most likely a regression, but not clear how much of one. We don't
> currently have benchmarks measuring this in v8 upstream, but I'd be surprised
if
> it didn't show up on the oortonline benchmark.
> > 
> > As mentioned on the bug, we could avoid removing the optimization entirely
by
> adding the following checks:
> > 
> > 1. %ArrayIteratorPrototype%.next is still the original value
> > 2. arrayLike object has fast elements
> > 3. arrayLike object's length property is a data property
> > 4. arrayLike object is not a JSProxy
> > 
> > But, it seemed like it probably wasn't worth it. Instead of a shortcut, this
> needs a more sound optimization strategy
> 
> Right, and probably that's easier in C++ than JavaScript.

Yeah, this is worth thinking about more later, but for now I'd like to get rid
of the observable broken-ness

[caitp, 2016-11-30 17:10:59]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2016-11-30 17:11:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 17:12:57]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480525859915570, "parent_rev":
"64c5391fa8c4681ba15f58f5609b8944a4594100", "commit_rev":
"75e4f521e605de7e7a62491f1666e636f2b727ce"}

[commit-bot: I haz the power, 2016-11-30 17:13:04]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-30 17:13:37]

Description was changed from

==========
[typedarrays] remove invalid optimization in NAMEConstructor()

Before, we were treating objects with the builtin ArrayValues iterator
method as array-like, where the iterator would iterate through to the
full length of the object.

This optimization was not sound, because it does not ensure that the
next method hasn't been modified. Even if it hasn't been modified,
it's entirely possible to be modified during iteration. Thus, this
optimization has been removed due to its observability.

BUG=v8:5699
R=littledan@chromium.org, cbruni@chromium.org
==========

to

==========
[typedarrays] remove invalid optimization in NAMEConstructor()

Before, we were treating objects with the builtin ArrayValues iterator
method as array-like, where the iterator would iterate through to the
full length of the object.

This optimization was not sound, because it does not ensure that the
next method hasn't been modified. Even if it hasn't been modified,
it's entirely possible to be modified during iteration. Thus, this
optimization has been removed due to its observability.

BUG=v8:5699
R=littledan@chromium.org, cbruni@chromium.org

Committed: https://crrev.com/77df8c67d9609ada3b7d79e8e6d33f198bbad5a1
Cr-Commit-Position: refs/heads/master@{#41394}
==========

[commit-bot: I haz the power, 2016-11-30 17:13:38]

Patchset 1 (id:??) landed as
https://crrev.com/77df8c67d9609ada3b7d79e8e6d33f198bbad5a1
Cr-Commit-Position: refs/heads/master@{#41394}

[enne (OOO), 2016-12-01 22:15:15]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2548583003/ by enne@chromium.org.

The reason for reverting is: Speculative revert for causing timeouts on Win
Debug gpu fyi bot

Nothing else looks even remotely relevant in the list of changes.
Will reland if this doesn't fix the issues.

BUG=670396.