[runtime] Fancify KeyAccumulator

[runtime] Fancify KeyAccumulator
Separately collect element keys from property keys to avoid slow
corner-cases. Partly deal with keys generated by Proxies.

BUG=chromium:536790
LOG=N

Committed: https://crrev.com/df8970a8c735014763454b4a6b8b25a19c742544
Cr-Commit-Position: refs/heads/master@{#31378}

[Camillo Bruni, 2015-10-13 13:49:25]

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

[Camillo Bruni, 2015-10-13 13:49:26]

PTAL
I still have a memory issue (as indicated by the comment).
Besides some obvious code duplications (CollectElementIndices) this
implementation should behave slightly better than the previous version.

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

https://codereview.chromium.org/1397063002/diff/20001/src/objects.cc#newcode6976
src/objects.cc:6976: elements_.Add(new List<uint32_t>());
I am not sure how these lists are freed. I presume that they are currently
leaked somehow, too much C++ magic going on.

https://codereview.chromium.org/1397063002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1397063002/diff/40001/src/objects.cc#newcode6976
src/objects.cc:6976: elements_.Add(new List<uint32_t>());
This will cause a memory leak. Since the sublists are not heap-allocated.
I am not sure how to properly address this, I'm still confused by how delete
works in this case.

https://codereview.chromium.org/1397063002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:14060: uint32_t offset) {
To be shared with ArrayConcat visitor in builtins.cc

[Camillo Bruni, 2015-10-15 09:46:25]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 09:46:31]

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

[Igor Sheludko, 2015-10-15 09:51:44]

https://codereview.chromium.org/1397063002/diff/80001/src/elements.h
File src/elements.h (right):

https://codereview.chromium.org/1397063002/diff/80001/src/elements.h#newcode36
src/elements.h:36: PropertyAttributes filter = NONE) = 0;
Please add a comment about filter parameter. How exactly does it filter out
entries? What does NONE mean?

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc#newcode6808
src/objects.cc:6808: // proxy, hence we skip the integer keys in |elements_|
since procies
s/procies/proxies/

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc#newcode6833
src/objects.cc:6833: }
DCHECK_EQ(index, length_);

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc#newcode6881
src/objects.cc:6881: }
Just to be sure we are not trying to add to the proxy-level:
DCHECK_LE(0, levelLength_);

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc#newcode1...
src/objects.cc:14123: uint32_t length = accessor->GetCapacity(*object,
object->elements());
I don't like exposing ElementsAccessor::GetCapacity(). It looks like the whole
CollectElementIndices() function should be another virtual method of 
ElementsAccessor's. WDYT?

https://codereview.chromium.org/1397063002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1397063002/diff/80001/src/objects.h#newcode3337
src/objects.h:3337: static void CopyElementKeysTo(Handle<Derived> dictionary,
Maybe CollectElementKeys is a better name.

https://codereview.chromium.org/1397063002/diff/80001/src/objects.h#newcode10676
src/objects.h:10676: class KeyAccumulator final BASE_EMBEDDED {
I think it became complex enough to have a comment.

https://codereview.chromium.org/1397063002/diff/80001/src/runtime/runtime-arr...
File src/runtime/runtime-array.cc (right):

https://codereview.chromium.org/1397063002/diff/80001/src/runtime/runtime-arr...
src/runtime/runtime-array.cc:209: // No need to separate protoype levels sicne
we only get numbers/element keys
s/sicne/since/

[commit-bot: I haz the power, 2015-10-15 10:09:03]

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

[commit-bot: I haz the power, 2015-10-15 10:09:04]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-10-16 09:38:02]

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1397063002/diff/80001/src/objects.cc#newcode1...
src/objects.cc:14123: uint32_t length = accessor->GetCapacity(*object,
object->elements());
On 2015/10/15 at 09:51:43, Igor Sheludko wrote:
> I don't like exposing ElementsAccessor::GetCapacity(). It looks like the whole
CollectElementIndices() function should be another virtual method of 
ElementsAccessor's. WDYT?

Indeed, I postponed it since I had so many other bugs to figure out ;). I'll
move it.

https://codereview.chromium.org/1397063002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1397063002/diff/80001/src/objects.h#newcode3337
src/objects.h:3337: static void CopyElementKeysTo(Handle<Derived> dictionary,
On 2015/10/15 at 09:51:44, Igor Sheludko wrote:
> Maybe CollectElementKeys is a better name.

sounds good.

https://codereview.chromium.org/1397063002/diff/80001/src/objects.h#newcode10676
src/objects.h:10676: class KeyAccumulator final BASE_EMBEDDED {
On 2015/10/15 at 09:51:44, Igor Sheludko wrote:
> I think it became complex enough to have a comment.

indeed :) I already added extensive comments.

[Camillo Bruni, 2015-10-16 12:02:09]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 12:02:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1397063002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1397063002/120001

[commit-bot: I haz the power, 2015-10-16 13:28:21]

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

[commit-bot: I haz the power, 2015-10-16 13:28:21]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-10-16 13:47:36]

PTAL again. Thanks for the detailed review.

[Igor Sheludko, 2015-10-19 13:11:20]

lgtm with nits:

https://codereview.chromium.org/1397063002/diff/120001/src/elements.h
File src/elements.h (right):

https://codereview.chromium.org/1397063002/diff/120001/src/elements.h#newcode201
src/elements.h:201: virtual uint32_t GetCapacity(JSObject* holder,
Spurious change.

https://codereview.chromium.org/1397063002/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1397063002/diff/120001/src/objects.cc#newcode...
src/objects.cc:7504: // make sure we have all the lengths collected
Make ... collected.

https://codereview.chromium.org/1397063002/diff/120001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1397063002/diff/120001/src/objects.h#newcode3349
src/objects.h:3349: 
Spurious change.

https://codereview.chromium.org/1397063002/diff/120001/src/objects.h#newcode1...
src/objects.h:10731: // does
Extra line break.

https://codereview.chromium.org/1397063002/diff/120001/src/objects.h#newcode1...
src/objects.h:10734: // For performance reasones the KeyAccumulator internally
separates integer
s/reasones/reasons/

https://codereview.chromium.org/1397063002/diff/120001/src/objects.h#newcode1...
src/objects.h:10740: // Only unique keys are kepy by the KeyAccumulator, strings
are stored in a
s/kepy/kept/

[Camillo Bruni, 2015-10-19 13:23:03]

Description was changed from

==========
[runtime] Fancify KeyAccumulator
Separately collect element keys from property keys to avoid slow
corner-cases. Partly deal with keys generated by Proxies.

BUG=
==========

to

==========
[runtime] Fancify KeyAccumulator
Separately collect element keys from property keys to avoid slow
corner-cases. Partly deal with keys generated by Proxies.

BUG=chromium/536790
LOG=N
==========

[Camillo Bruni, 2015-10-19 13:23:21]

Description was changed from

==========
[runtime] Fancify KeyAccumulator
Separately collect element keys from property keys to avoid slow
corner-cases. Partly deal with keys generated by Proxies.

BUG=chromium/536790
LOG=N
==========

to

==========
[runtime] Fancify KeyAccumulator
Separately collect element keys from property keys to avoid slow
corner-cases. Partly deal with keys generated by Proxies.

BUG=chromium:536790
LOG=N
==========

[Camillo Bruni, 2015-10-19 13:23:30]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-10-19 13:23:30]

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

[commit-bot: I haz the power, 2015-10-19 13:23:34]

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

[commit-bot: I haz the power, 2015-10-19 13:47:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-19 13:47:48]

Patchset 8 (id:??) landed as
https://crrev.com/df8970a8c735014763454b4a6b8b25a19c742544
Cr-Commit-Position: refs/heads/master@{#31378}