[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org

Committed: https://crrev.com/3088979d5cd23fddb1fcbe0cd1a51673608e9761
Cr-Commit-Position: refs/heads/master@{#34754}

[caitp (gmail), 2016-03-10 04:34:09]

Patchset #1 (id:1) has been deleted

[caitp (gmail), 2016-03-10 07:15:25]

PTAL --- fixes some minor issues in ElementsAccessor.

I'm not sure if the PrependElementIndices thing is actually needed for any
reason, and it's probably possible to write this stuff more cleanly (the CRTP
stuff is hard to read, especially at this hour)

[caitp (gmail), 2016-03-10 07:50:32]

On 2016/03/10 07:15:25, caitp wrote:
> PTAL --- fixes some minor issues in ElementsAccessor.
> 
> I'm not sure if the PrependElementIndices thing is actually needed for any
> reason, and it's probably possible to write this stuff more cleanly (the CRTP
> stuff is hard to read, especially at this hour)

ehh, thisisnt ready yet, nvm

[Camillo Bruni, 2016-03-10 09:15:14]

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

https://codereview.chromium.org/1778023004/diff/40001/src/elements.cc#newcode...
src/elements.cc:2345: keys->SortCurrentElementsList();
I would prefer a base implementation in SloppyArgumentsElementsAccessor which
forwards the non-fixed-arguments part to the ArgumentsAccessor template
argument.
IMO that would make the implementation a bit cleaner than just duplicating the
whole dictionary implementations again, WDYT?

I'm also not fully sure why you re-implemented the dictionary part slightly
different than the corresponding
DictionaryElementsAccessor::CollectElementIndicesImpl, AFAIK you could directly
call those methods on the dictionary part.

https://codereview.chromium.org/1778023004/diff/40001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-4825.js (right):

https://codereview.chromium.org/1778023004/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-4825.js:45: })();
Nice tests!
Could you create a second set with a lower index (3, instead of 10000), to test
FastSloppyArguments (that is HOLEY secondary backing_store to the arguments
object instead of a DICTIONARY)?
Like that we should be more or less safe that we hit all paths...

[caitp (gmail), 2016-03-10 16:58:34]

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

https://codereview.chromium.org/1778023004/diff/40001/src/elements.cc#newcode...
src/elements.cc:2345: keys->SortCurrentElementsList();
On 2016/03/10 09:15:14, cbruni wrote:
> I would prefer a base implementation in SloppyArgumentsElementsAccessor which
> forwards the non-fixed-arguments part to the ArgumentsAccessor template
> argument.
> IMO that would make the implementation a bit cleaner than just duplicating the
> whole dictionary implementations again, WDYT?
> 
> I'm also not fully sure why you re-implemented the dictionary part slightly
> different than the corresponding
> DictionaryElementsAccessor::CollectElementIndicesImpl, AFAIK you could
directly
> call those methods on the dictionary part.

Done.

https://codereview.chromium.org/1778023004/diff/40001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-4825.js (right):

https://codereview.chromium.org/1778023004/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-4825.js:45: })();
On 2016/03/10 09:15:14, cbruni wrote:
> Nice tests!
> Could you create a second set with a lower index (3, instead of 10000), to
test
> FastSloppyArguments (that is HOLEY secondary backing_store to the arguments
> object instead of a DICTIONARY)?
> Like that we should be more or less safe that we hit all paths...

Done

[Camillo Bruni, 2016-03-10 17:08:51]

lgtm, nice!

[caitp (gmail), 2016-03-10 17:31:18]

Description was changed from

==========
[runtime] fix getting elements from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org, adamk@chromium.org
==========

to

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2016-03-10 20:37:12]

Thanks --- I see some draft messages waiting to be sent still.

[caitp (gmail), 2016-03-11 14:54:51]

caitpotter88@gmail.com changed reviewers:
+ verwaest@chromium.org

[Camillo Bruni, 2016-03-14 15:59:40]

humm I think these are your own draft messages no? :D
At least from my part this is LGTM :) (and toon has nothing to add either
AFAICT).

[caitp (gmail), 2016-03-14 16:05:15]

On 2016/03/14 15:59:40, cbruni wrote:
> humm I think these are your own draft messages no? :D
> At least from my part this is LGTM :) (and toon has nothing to add either
> AFAICT).

Nope, I have no drafts on this CL, unless rietveld is doing something weird atm
(which would not be totally surprising).

Still need a sign off from an OWNER, anyways.

[Jakob Kummerow, 2016-03-14 16:09:31]

rubberstamp LGTM

[caitp (gmail), 2016-03-14 16:12:58]

Description was changed from

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org, adamk@chromium.org
==========

to

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org
==========

[caitp (gmail), 2016-03-14 16:13:04]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2016-03-14 16:13:07]

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

[caitp (gmail), 2016-03-14 16:13:30]

On 2016/03/14 16:09:31, Jakob wrote:
> rubberstamp LGTM

Thanks!

[commit-bot: I haz the power, 2016-03-14 17:09:27]

Description was changed from

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org
==========

to

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org
==========

[commit-bot: I haz the power, 2016-03-14 17:09:28]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-03-14 17:10:59]

Description was changed from

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org
==========

to

==========
[runtime] fix getting element keys from SLOW_SLOPPY_ARGUMENTS_ELEMENTS

BUG=v8:4825
LOG=N
R=jkummerow@chromium.org, cbruni@chromium.org

Committed: https://crrev.com/3088979d5cd23fddb1fcbe0cd1a51673608e9761
Cr-Commit-Position: refs/heads/master@{#34754}
==========

[commit-bot: I haz the power, 2016-03-14 17:11:01]

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