[proxies] allow duplicate keys for [[OwnPropertyKeys]] trap.

[proxies] allow duplicate keys for [[OwnPropertyKeys]] trap.

BUG=v8:4724, v8:1543
LOG=N

Committed: https://crrev.com/07d05dddceab37f4da8d382e3fe60e90e02b99e5
Cr-Commit-Position: refs/heads/master@{#33747}

[Camillo Bruni, 2016-02-04 11:59:06]

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

[Camillo Bruni, 2016-02-04 11:59:07]

PTAL

https://codereview.chromium.org/1668853002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1668853002/diff/1/src/objects.cc#newcode8884
src/objects.cc:8884: if (found == nullptr || *found == kGone) {
I didn't fully check how IdentityMaps work, but there seems to be no way to
directly get the size out.

https://codereview.chromium.org/1668853002/diff/1/src/objects.cc#newcode8900
src/objects.cc:8900: // 17b. Remove key from uncheckedResultKeys.
I read this now as remove all occurences of key from uncheckedResultKeys. Which
is not fully clear/obvious.

[caitp (gmail), 2016-02-04 14:46:15]

On 2016/02/04 11:59:07, cbruni wrote:
> PTAL
> 
> https://codereview.chromium.org/1668853002/diff/1/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/1668853002/diff/1/src/objects.cc#newcode8884
> src/objects.cc:8884: if (found == nullptr || *found == kGone) {
> I didn't fully check how IdentityMaps work, but there seems to be no way to
> directly get the size out.
> 
> https://codereview.chromium.org/1668853002/diff/1/src/objects.cc#newcode8900
> src/objects.cc:8900: // 17b. Remove key from uncheckedResultKeys.
> I read this now as remove all occurences of key from uncheckedResultKeys.
Which
> is not fully clear/obvious.

the invariant fix looks good

[Jakob Kummerow, 2016-02-04 15:37:27]

LGTM, just nits.

https://codereview.chromium.org/1668853002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1668853002/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1591: isolate, keys, JSReceiver::GetKeys(from,
KeyCollectionType::OWN_ONLY,
nit: Why don't you s/KeyCollectionType::// here and elsewhere to trade
explicitness for conciseness? Up to you.

https://codereview.chromium.org/1668853002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1668853002/diff/20001/src/objects.cc#newcode8883
src/objects.cc:8883: int* found = unchecked_result_keys.Find(key);
nit: (premature?) performance optimization: Get can replace the Find/Set pair,
like so:

    int* entry = unchecked_result_keys.Get(key);
    if (*entry != kPresent) {
      *entry = kPresent;
      unchecked_result_keys_size++;
    }

https://codereview.chromium.org/1668853002/diff/20001/src/objects.cc#newcode8944
src/objects.cc:8944: GetKeys_Internal(isolate, object, object, type, filter,
&accumulator),
We could consider adding public type() and filter() getters to the
KeyAccumulator, and then pass around only the accumulator to all these helper
functions. If we do that it should probably be a separate CL though.

[Camillo Bruni, 2016-02-04 17:13:53]

Description was changed from

==========
[proxies] allow duplicate keys for [[OwnPropertyKeys]] trap.

BUG=v8:4724, v8:1543
==========

to

==========
[proxies] allow duplicate keys for [[OwnPropertyKeys]] trap.

BUG=v8:4724, v8:1543
LOG=N
==========

[Camillo Bruni, 2016-02-04 17:13:56]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-02-04 17:13:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1668853002/#ps40001
(title: "addressing nits")

[commit-bot: I haz the power, 2016-02-04 17:14:04]

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

[Camillo Bruni, 2016-02-04 17:14:32]

https://codereview.chromium.org/1668853002/diff/20001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1668853002/diff/20001/src/builtins.cc#newcode...
src/builtins.cc:1591: isolate, keys, JSReceiver::GetKeys(from,
KeyCollectionType::OWN_ONLY,
On 2016/02/04 at 15:37:27, Jakob wrote:
> nit: Why don't you s/KeyCollectionType::// here and elsewhere to trade
explicitness for conciseness? Up to you.

uh right :)

https://codereview.chromium.org/1668853002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1668853002/diff/20001/src/objects.cc#newcode8944
src/objects.cc:8944: GetKeys_Internal(isolate, object, object, type, filter,
&accumulator),
On 2016/02/04 at 15:37:27, Jakob wrote:
> We could consider adding public type() and filter() getters to the
KeyAccumulator, and then pass around only the accumulator to all these helper
functions. If we do that it should probably be a separate CL though.

right, I think that would be a nice little cleanup task.

[commit-bot: I haz the power, 2016-02-04 17:28:00]

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

[commit-bot: I haz the power, 2016-02-04 17:28:01]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/10549)

[Camillo Bruni, 2016-02-04 17:50:09]

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

[Camillo Bruni, 2016-02-04 17:50:09]

Yang, could you lgtm the mechanical changes under debug/* ? thanks.

[Yang, 2016-02-04 17:52:52]

On 2016/02/04 17:50:09, cbruni wrote:
> Yang, could you lgtm the mechanical changes under debug/* ? thanks.

lgtm

[Camillo Bruni, 2016-02-04 17:53:06]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2016-02-04 17:53:17]

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

[Camillo Bruni, 2016-02-04 17:53:18]

grazie.

[commit-bot: I haz the power, 2016-02-04 17:55:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-04 17:55:47]

Description was changed from

==========
[proxies] allow duplicate keys for [[OwnPropertyKeys]] trap.

BUG=v8:4724, v8:1543
LOG=N
==========

to

==========
[proxies] allow duplicate keys for [[OwnPropertyKeys]] trap.

BUG=v8:4724, v8:1543
LOG=N

Committed: https://crrev.com/07d05dddceab37f4da8d382e3fe60e90e02b99e5
Cr-Commit-Position: refs/heads/master@{#33747}
==========

[commit-bot: I haz the power, 2016-02-04 17:55:47]

Patchset 3 (id:??) landed as
https://crrev.com/07d05dddceab37f4da8d382e3fe60e90e02b99e5
Cr-Commit-Position: refs/heads/master@{#33747}