Cleanup adding elements and in particular dictionary elements

Cleanup adding elements and in particular dictionary elements

BUG=v8:4137
LOG=n

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

[Toon Verwaest, 2015-06-22 20:26:58]

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

[Toon Verwaest, 2015-06-22 20:26:59]

ptal

[Jakob Kummerow, 2015-06-23 12:05:14]

LGTM with comments.

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

https://codereview.chromium.org/1196163005/diff/40001/src/elements.cc#newcode992
src/elements.cc:992: if (IsFastSmiElementsKind(KindTraits::Kind)) {
I wonder if it would be useful to validate fast object elements too. At least
two things could be checked: 
(1) for FAST_ELEMENTS, there are no holes; 
(2) holey or not, all non-holes are either Smis or valid heap objects (roughly
"o->IsHeapObject() && HeapObject::cast(o)->map()->IsMap()")

Can leave it as is for now, though. Adding more checks would make pretty much
every single Debug test run slower, and currently we have no reason to suspect
bugs in this area.

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

https://codereview.chromium.org/1196163005/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12499: length = index + 1;
Who guarantees that index != kMaxUint32?

https://codereview.chromium.org/1196163005/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12607: if (IsFastSmiOrObjectElementsKind(kind))
EnsureWritableFastElements(object);
Isn't it unnecessary to do this if we'll decide to handle_slow later on? I.e.,
wouldn't the following sequence make more sense:

    capacity = static_cast<uint32_t>(object->elements()->length());
    handle_slow =
        object->ShouldConvertToSlowElements(capacity, index, &new_capacity);
    if (handle_slow) {
      NormalizeElements(object)
    } else if (IsFastSimOrObjectElementsKind(kind)) {
      EnsureWritableFastElements(object);
    }

[Toon Verwaest, 2015-06-23 12:28:17]

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

https://codereview.chromium.org/1196163005/diff/40001/src/elements.cc#newcode992
src/elements.cc:992: if (IsFastSmiElementsKind(KindTraits::Kind)) {
On 2015/06/23 12:05:14, Jakob wrote:
> I wonder if it would be useful to validate fast object elements too. At least
> two things could be checked: 
> (1) for FAST_ELEMENTS, there are no holes; 
> (2) holey or not, all non-holes are either Smis or valid heap objects (roughly
> "o->IsHeapObject() && HeapObject::cast(o)->map()->IsMap()")
> 
> Can leave it as is for now, though. Adding more checks would make pretty much
> every single Debug test run slower, and currently we have no reason to suspect
> bugs in this area.

True. Can investigate later. Probably it makes more sense to just check the
stored value ...

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

https://codereview.chromium.org/1196163005/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12499: length = index + 1;
On 2015/06/23 12:05:14, Jakob wrote:
> Who guarantees that index != kMaxUint32?

We shouldn't be in the AddDataElement path altogether if the index wasn't an
"element".

https://codereview.chromium.org/1196163005/diff/40001/src/objects.cc#newcode1...
src/objects.cc:12607: if (IsFastSmiOrObjectElementsKind(kind))
EnsureWritableFastElements(object);
On 2015/06/23 12:05:14, Jakob wrote:
> Isn't it unnecessary to do this if we'll decide to handle_slow later on? I.e.,
> wouldn't the following sequence make more sense:
> 
>     capacity = static_cast<uint32_t>(object->elements()->length());
>     handle_slow =
>         object->ShouldConvertToSlowElements(capacity, index, &new_capacity);
>     if (handle_slow) {
>       NormalizeElements(object)
>     } else if (IsFastSimOrObjectElementsKind(kind)) {
>       EnsureWritableFastElements(object);
>     }

Done.

[Toon Verwaest, 2015-06-23 12:28:59]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2015-06-23 12:28:59]

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/1196163005/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-06-23 12:29:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1196163005/60001

[commit-bot: I haz the power, 2015-06-23 12:37:45]

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

[commit-bot: I haz the power, 2015-06-23 12:37:46]

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

[Toon Verwaest, 2015-06-23 12:49:24]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2015-06-23 12:49:25]

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/1196163005/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 12:49:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1196163005/80001

[commit-bot: I haz the power, 2015-06-23 13:35:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-23 13:35:26]

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