[builtins] take slow path in IsConcatSpreadable if proxy in prototype

[builtins] take slow path in IsConcatSpreadable if proxy in prototype

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

Committed: https://crrev.com/122a9b7af02606dae558336082ab139a87eba39d
Cr-Commit-Position: refs/heads/master@{#37987}

[caitp, 2016-07-08 21:04:50]

PTAL

(this probably hurts some benchmarks pretty badly due to checking the entire
prototype chain in IsConcatSpreadable() every time, I'm not sure the best way to
speed this up yet)

[Dan Ehrenberg, 2016-07-08 21:17:47]

On 2016/07/08 at 21:04:50, caitp wrote:
> PTAL
> 
> (this probably hurts some benchmarks pretty badly due to checking the entire
prototype chain in IsConcatSpreadable() every time, I'm not sure the best way to
speed this up yet)

Yeah, I'm wondering what the perf penalty is. Some benchmarks that are somewhat
sensitive to concat performance are (I believe) stanford-crypto-pbkdf2 and
early-boyer; Camillo may have better information about which benchmarks actually
hit the Slow_ArrayConcat path. Or you could start with a microbenchmark to get
an upper bound on the penalty.

Maybe you could somehow speed up the check for certain kinds of objects. For
example, if the IsConcatSpreadable lookup chain is intact, and the obj
IsSimpleArray, then we're all good and no need to go look up the prototype chain
(the Fast_ArrayConcat path's continued correctness depends on this, and I
believe it's true).

[Camillo Bruni, 2016-07-11 08:16:40]

On 2016/07/08 at 21:17:47, littledan wrote:

Can you try micro-benchmarking with a dictionary-elements array?
I think that's one of the easy cases we do not support on the fast path, I doubt
that more exotic objects will be a real issue in the wild.

[neis, 2016-07-11 09:25:19]

https://codereview.chromium.org/2131383002/diff/40001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2131383002/diff/40001/src/builtins.cc#newcode...
src/builtins.cc:1186: PrototypeIterator::END_AT_NON_HIDDEN);
This must be END_AT_NULL.

https://codereview.chromium.org/2131383002/diff/40001/src/builtins.cc#newcode...
src/builtins.cc:1197: HasProxyInPrototype(isolate, JSReceiver::cast(*obj))) {
I think it makes sense to move the proxy check into
IsIsConcatSpreadableLookupChainIntact.

https://codereview.chromium.org/2131383002/diff/40001/test/mjsunit/es6/array-...
File test/mjsunit/es6/array-concat.js (right):

https://codereview.chromium.org/2131383002/diff/40001/test/mjsunit/es6/array-...
test/mjsunit/es6/array-concat.js:894: assertThrows(function() { [].concat({
__proto__: p.proxy }); }, TypeError);
Unfortunately the test file is a bit screwed up: this version passes even
without your code change.  The reason is that the protector thingie gets already
invalidated in one of the earlier test functions, so the problematic path won't
get tested.  Not sure right now how to fix this other than moving the new test
to the top.

[caitp, 2016-07-11 11:12:09]

https://codereview.chromium.org/2131383002/diff/40001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/2131383002/diff/40001/src/builtins.cc#newcode...
src/builtins.cc:1186: PrototypeIterator::END_AT_NON_HIDDEN);
On 2016/07/11 09:25:19, neis wrote:
> This must be END_AT_NULL.

I have no idea why it's not END_AT_NULL, I could have sworn I didn't add this
parameter o_O. Will fix in the next patch.

https://codereview.chromium.org/2131383002/diff/40001/src/builtins.cc#newcode...
src/builtins.cc:1197: HasProxyInPrototype(isolate, JSReceiver::cast(*obj))) {
On 2016/07/11 09:25:19, neis wrote:
> I think it makes sense to move the proxy check into
> IsIsConcatSpreadableLookupChainIntact.
> 

Acknowledged, however this would change the prototype of
IsIsConcatSpreadableLookupChainIntact, which might get messy

https://codereview.chromium.org/2131383002/diff/40001/test/mjsunit/es6/array-...
File test/mjsunit/es6/array-concat.js (right):

https://codereview.chromium.org/2131383002/diff/40001/test/mjsunit/es6/array-...
test/mjsunit/es6/array-concat.js:894: assertThrows(function() { [].concat({
__proto__: p.proxy }); }, TypeError);
On 2016/07/11 09:25:19, neis wrote:
> Unfortunately the test file is a bit screwed up: this version passes even
> without your code change.  The reason is that the protector thingie gets
already
> invalidated in one of the earlier test functions, so the problematic path
won't
> get tested.  Not sure right now how to fix this other than moving the new test
> to the top.

Thanks for the info, will do

[caitp, 2016-07-11 12:02:09]

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

[commit-bot: I haz the power, 2016-07-11 12:02:47]

Dry run: 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-07-11 12:02:48]

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

[commit-bot: I haz the power, 2016-07-11 12:02:49]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[caitp, 2016-07-11 13:19:13]

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

[commit-bot: I haz the power, 2016-07-11 13:19:22]

Dry run: 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-07-11 13:19:23]

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

[commit-bot: I haz the power, 2016-07-11 13:19:24]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Camillo Bruni, 2016-07-13 13:02:15]

lgtm with nits

https://codereview.chromium.org/2131383002/diff/60001/test/mjsunit/es6/array-...
File test/mjsunit/es6/array-concat.js (right):

https://codereview.chromium.org/2131383002/diff/60001/test/mjsunit/es6/array-...
test/mjsunit/es6/array-concat.js:896: })();
Could you add the same test in a separate file so we are sure that the path also
works with a valid and an invalidated IsConcatSpreadable protector?

[Dan Ehrenberg, 2016-07-13 18:42:16]

On 2016/07/13 13:02:15, Camillo Bruni wrote:
> lgtm with nits

Do we have benchmark results yet? Should we maybe wait on those before
submitting, or do we just want to throw it at the perf bots?

[caitp, 2016-07-22 16:28:40]

On 2016/07/13 18:42:16, Dan Ehrenberg wrote:
> On 2016/07/13 13:02:15, Camillo Bruni wrote:
> > lgtm with nits
> 
> Do we have benchmark results yet? Should we maybe wait on those before
> submitting, or do we just want to throw it at the perf bots?

Sorry, it's taken forever to answer this due to working on that A.p.includes.

Personally, I'm fine with trying the bots and reverting if we see a negative
trend. I don't expect I will personally get conclusive results from running
benchmarks manually anyways, without the google-internal tooling.

Not sure if this patch still applies clean, though, so I'll send a rebase.

[caitp, 2016-07-22 18:18:04]

The CQ bit was checked by caitp@igalia.com

[caitp, 2016-07-22 18:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2131383002/#ps80001
(title: "rebased")

[commit-bot: I haz the power, 2016-07-22 18:18:12]

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-07-22 18:42:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-22 18:43:29]

Description was changed from

==========
[builtins] take slow path in IsConcatSpreadable if proxy in prototype

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

to

==========
[builtins] take slow path in IsConcatSpreadable if proxy in prototype

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

Committed: https://crrev.com/122a9b7af02606dae558336082ab139a87eba39d
Cr-Commit-Position: refs/heads/master@{#37987}
==========

[commit-bot: I haz the power, 2016-07-22 18:43:31]

Patchset 5 (id:??) landed as
https://crrev.com/122a9b7af02606dae558336082ab139a87eba39d
Cr-Commit-Position: refs/heads/master@{#37987}

[neis, 2016-07-25 07:41:15]

The test file is still broken, i.e., it passes without your code changes.

[caitp, 2016-07-25 12:11:31]

On 2016/07/25 07:41:15, neis wrote:
> The test file is still broken, i.e., it passes without your code changes.

Oop. Well, I can send a followup to fix the tests, its fine to revert first