[esnext] use map instance_descriptors() when possible in Object.values/entries()

[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the map changes during a call to an accessor property, resort to a slower property lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, verwaest@chromium.org, adamk@chromium.org

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

[caitp (gmail), 2016-03-01 18:30:22]

A little bit of perf tuning for some (I imagine) common cases. It provides a
measurable improvement in some common cases, outperforming lodash for the same
tests (on average)

[caitp (gmail), 2016-03-01 18:40:32]

On 2016/03/01 18:30:22, caitp wrote:
> A little bit of perf tuning for some (I imagine) common cases. It provides a
> measurable improvement in some common cases, outperforming lodash for the same
> tests (on average)

by "measurable", I'm talking about 10-15% bumps (based on local js-perf-test
results for those enum-cache-able cases)

[Camillo Bruni, 2016-03-01 20:50:14]

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

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8856
src/objects.cc:8856: did_use_enum_cache = true;
I've got a long-term project running that eventually will handle this directly
in the KeyAccumulator.
See https://codereview.chromium.org/1707743002

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8876
src/objects.cc:8876: object->map()->EnumLength() == kInvalidEnumCacheSentinel))
{
why the redundant check for the enum_length again here?
did_use_enum_cache implies (object->map()->EnumLength() !=
kInvalidEnumCacheSentinel)?
and if did_use_enum_cache = false you don't care about the enum_length anyway.

[caitp (gmail), 2016-03-01 21:02:40]

thanks for the look!

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

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8856
src/objects.cc:8856: did_use_enum_cache = true;
On 2016/03/01 20:50:13, cbruni wrote:
> I've got a long-term project running that eventually will handle this directly
> in the KeyAccumulator.
> See https://codereview.chromium.org/1707743002

On 2016/03/01 20:50:13, cbruni wrote:
> I've got a long-term project running that eventually will handle this directly
> in the KeyAccumulator.
> See https://codereview.chromium.org/1707743002

It's not just for getting the property keys that I want to use this --- I'd like
to be able to use this to skip the [[GetOwnProperty]] call on line 8878, if I
can guarantee that the enum status hasn't changed at any point during the
algorithm.

I think that makes sense, what do you think?

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8876
src/objects.cc:8876: object->map()->EnumLength() == kInvalidEnumCacheSentinel))
{
On 2016/03/01 20:50:13, cbruni wrote:
> why the redundant check for the enum_length again here?
> did_use_enum_cache implies (object->map()->EnumLength() !=
> kInvalidEnumCacheSentinel)?
> and if did_use_enum_cache = false you don't care about the enum_length anyway.

On line 8885, we [[GET]] the property, which can replace or add descriptors and
invalidate the enum cache. So, the idea here is, the work can be skipped if the
current EnumLength() is the sentinel. I've cleaned this up a little bit in the
newest patchset

[caitp (gmail), 2016-03-01 21:12:25]

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

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8876
src/objects.cc:8876: object->map()->EnumLength() == kInvalidEnumCacheSentinel))
{
On 2016/03/01 21:02:40, caitp wrote:
> On 2016/03/01 20:50:13, cbruni wrote:
> > why the redundant check for the enum_length again here?
> > did_use_enum_cache implies (object->map()->EnumLength() !=
> > kInvalidEnumCacheSentinel)?
> > and if did_use_enum_cache = false you don't care about the enum_length
anyway.
> 
> On line 8885, we [[GET]] the property, which can replace or add descriptors
and
> invalidate the enum cache. So, the idea here is, the work can be skipped if
the
> current EnumLength() is the sentinel. I've cleaned this up a little bit in the
> newest patchset

***work can be skipped if the EnumLength() is __not__ the sentinel value, oops
:<

[Camillo Bruni, 2016-03-02 09:21:51]

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

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8856
src/objects.cc:8856: did_use_enum_cache = true;
On 2016/03/01 at 21:02:40, caitp wrote:
> On 2016/03/01 20:50:13, cbruni wrote:
> > I've got a long-term project running that eventually will handle this
directly
> > in the KeyAccumulator.
> > See https://codereview.chromium.org/1707743002
> 
> On 2016/03/01 20:50:13, cbruni wrote:
> > I've got a long-term project running that eventually will handle this
directly
> > in the KeyAccumulator.
> > See https://codereview.chromium.org/1707743002
> 
> It's not just for getting the property keys that I want to use this --- I'd
like to be able to use this to skip the [[GetOwnProperty]] call on line 8878, if
I can guarantee that the enum status hasn't changed at any point during the
algorithm.
> 
> I think that makes sense, what do you think?

Right, probably we could make the new key accumulator thingy store whether it
just used the enum cache (aka SimpleEnum case). Then later on you just have to
check whether the map changed or not. We do the same in Object.assign
(FastAssign in builtins.cc).

https://codereview.chromium.org/1751643003/diff/1/src/objects.cc#newcode8876
src/objects.cc:8876: object->map()->EnumLength() == kInvalidEnumCacheSentinel))
{
On 2016/03/01 at 21:12:24, caitp wrote:
> On 2016/03/01 21:02:40, caitp wrote:
> > On 2016/03/01 20:50:13, cbruni wrote:
> > > why the redundant check for the enum_length again here?
> > > did_use_enum_cache implies (object->map()->EnumLength() !=
> > > kInvalidEnumCacheSentinel)?
> > > and if did_use_enum_cache = false you don't care about the enum_length
anyway.
> > 
> > On line 8885, we [[GET]] the property, which can replace or add descriptors
and
> > invalidate the enum cache. So, the idea here is, the work can be skipped if
the
> > current EnumLength() is the sentinel. I've cleaned this up a little bit in
the
> > newest patchset
> 
> ***work can be skipped if the EnumLength() is __not__ the sentinel value, oops
:<

I realised this once I fell asleep ;) too tired to see the common pattern.

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

https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8845
src/objects.cc:8845: object->IsJSObject() &&
You could probably include the OnlyHasSimpleProperties() check here.
Since you don't want to generate the enum cache for the non-simple case
(intercptors + access checks will be omitted).

https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8859
src/objects.cc:8859: } else if (maybe_enum_cache && object->HasFastProperties())
{
With the above OnlyHasSimpleProperties check the HasFastProperties becomes
redundant here.

https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8894
src/objects.cc:8894: object->map()->EnumLength() == kInvalidEnumCacheSentinel) {
How about just doing a map check (see FastAssign in builtins.cc), that would
probably make the intentions even clearer.

[caitp (gmail), 2016-03-02 11:53:21]

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

https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8894
src/objects.cc:8894: object->map()->EnumLength() == kInvalidEnumCacheSentinel) {
On 2016/03/02 09:21:51, cbruni wrote:
> How about just doing a map check (see FastAssign in builtins.cc), that would
> probably make the intentions even clearer.

my reasoning for this method is that, accessors can mutate the object, but still
use the enum cache if nothing was deleted or descriptors replaced. so some map
changes should be ok---but if it doesn't really work that way, of course

[Camillo Bruni, 2016-03-02 17:50:07]

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

https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8894
src/objects.cc:8894: object->map()->EnumLength() == kInvalidEnumCacheSentinel) {
On 2016/03/02 at 11:53:21, caitp wrote:
> On 2016/03/02 09:21:51, cbruni wrote:
> > How about just doing a map check (see FastAssign in builtins.cc), that would
> > probably make the intentions even clearer.
> 
> my reasoning for this method is that, accessors can mutate the object, but
still use the enum cache if nothing was deleted or descriptors replaced. so some
map changes should be ok---but if it doesn't really work that way, of course

IMO that would be partially correct. But it could be during the [[Get]] call
above you might change the map *and* initialize the enum-cache, in which case
you probably don't want to rely on the fast path either.

BTW check again the Object.assign fast-path, which uses the same pattern.
I think you should try to directly collect and read the properties if you have a
simple object (you can directly iterate over the enum cache and use the indices
in there for fast-mode objects). This would avoid using the lookup iterator (but
imply an explicit map check). And if you do that, I would factor it out (like in
Object.assign) into a Fast_GetOwnValuesOrEntries.
A follow-up CL might be better suited for this, if you want :).

[caitp (gmail), 2016-03-02 18:17:28]

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

https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8894
src/objects.cc:8894: object->map()->EnumLength() == kInvalidEnumCacheSentinel) {
On 2016/03/02 17:50:07, cbruni wrote:
> On 2016/03/02 at 11:53:21, caitp wrote:
> > On 2016/03/02 09:21:51, cbruni wrote:
> > > How about just doing a map check (see FastAssign in builtins.cc), that
would
> > > probably make the intentions even clearer.
> > 
> > my reasoning for this method is that, accessors can mutate the object, but
> still use the enum cache if nothing was deleted or descriptors replaced. so
some
> map changes should be ok---but if it doesn't really work that way, of course
> 
> IMO that would be partially correct. But it could be during the [[Get]] call
> above you might change the map *and* initialize the enum-cache, in which case
> you probably don't want to rely on the fast path either.
> 
> BTW check again the Object.assign fast-path, which uses the same pattern.
> I think you should try to directly collect and read the properties if you have
a
> simple object (you can directly iterate over the enum cache and use the
indices
> in there for fast-mode objects). This would avoid using the lookup iterator
(but
> imply an explicit map check). And if you do that, I would factor it out (like
in
> Object.assign) into a Fast_GetOwnValuesOrEntries.
> A follow-up CL might be better suited for this, if you want :).

Maybe I'll do that in a follow-up just so I can seem more busy while I'm billing
hours for this stuff (just kidding Andy, pay no attention to that!)

Sounds good to me, I should have an update in a few hours incorporating that

[caitp (gmail), 2016-03-02 19:24:31]

On 2016/03/02 18:17:28, caitp wrote:
> https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8894
> src/objects.cc:8894: object->map()->EnumLength() == kInvalidEnumCacheSentinel)
{
> On 2016/03/02 17:50:07, cbruni wrote:
> > On 2016/03/02 at 11:53:21, caitp wrote:
> > > On 2016/03/02 09:21:51, cbruni wrote:
> > > > How about just doing a map check (see FastAssign in builtins.cc), that
> would
> > > > probably make the intentions even clearer.
> > > 
> > > my reasoning for this method is that, accessors can mutate the object, but
> > still use the enum cache if nothing was deleted or descriptors replaced. so
> some
> > map changes should be ok---but if it doesn't really work that way, of course
> > 
> > IMO that would be partially correct. But it could be during the [[Get]] call
> > above you might change the map *and* initialize the enum-cache, in which
case
> > you probably don't want to rely on the fast path either.
> > 
> > BTW check again the Object.assign fast-path, which uses the same pattern.
> > I think you should try to directly collect and read the properties if you
have
> a
> > simple object (you can directly iterate over the enum cache and use the
> indices
> > in there for fast-mode objects). This would avoid using the lookup iterator
> (but
> > imply an explicit map check). And if you do that, I would factor it out
(like
> in
> > Object.assign) into a Fast_GetOwnValuesOrEntries.
> > A follow-up CL might be better suited for this, if you want :).
> 
> Maybe I'll do that in a follow-up just so I can seem more busy while I'm
billing
> hours for this stuff (just kidding Andy, pay no attention to that!)
> 
> Sounds good to me, I should have an update in a few hours incorporating that

I've made the suggested changes, doesn't seem to make a huge difference
(although I'm not sure the difference is an improvement)

[caitp (gmail), 2016-03-03 04:12:59]

On 2016/03/02 19:24:31, caitp wrote:
> On 2016/03/02 18:17:28, caitp wrote:
> > https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc
> > File src/objects.cc (right):
> > 
> >
>
https://codereview.chromium.org/1751643003/diff/40001/src/objects.cc#newcode8894
> > src/objects.cc:8894: object->map()->EnumLength() ==
kInvalidEnumCacheSentinel)
> {
> > On 2016/03/02 17:50:07, cbruni wrote:
> > > On 2016/03/02 at 11:53:21, caitp wrote:
> > > > On 2016/03/02 09:21:51, cbruni wrote:
> > > > > How about just doing a map check (see FastAssign in builtins.cc), that
> > would
> > > > > probably make the intentions even clearer.
> > > > 
> > > > my reasoning for this method is that, accessors can mutate the object,
but
> > > still use the enum cache if nothing was deleted or descriptors replaced.
so
> > some
> > > map changes should be ok---but if it doesn't really work that way, of
course
> > > 
> > > IMO that would be partially correct. But it could be during the [[Get]]
call
> > > above you might change the map *and* initialize the enum-cache, in which
> case
> > > you probably don't want to rely on the fast path either.
> > > 
> > > BTW check again the Object.assign fast-path, which uses the same pattern.
> > > I think you should try to directly collect and read the properties if you
> have
> > a
> > > simple object (you can directly iterate over the enum cache and use the
> > indices
> > > in there for fast-mode objects). This would avoid using the lookup
iterator
> > (but
> > > imply an explicit map check). And if you do that, I would factor it out
> (like
> > in
> > > Object.assign) into a Fast_GetOwnValuesOrEntries.
> > > A follow-up CL might be better suited for this, if you want :).
> > 
> > Maybe I'll do that in a follow-up just so I can seem more busy while I'm
> billing
> > hours for this stuff (just kidding Andy, pay no attention to that!)
> > 
> > Sounds good to me, I should have an update in a few hours incorporating that
> 
> I've made the suggested changes, doesn't seem to make a huge difference
> (although I'm not sure the difference is an improvement)

Last patchset is a trial to see if some of the advantages of the descriptor
array walking can still be used when elements are present, seems to work out
okay: http://jsperf.com/lodash-values-vs-object-values/4 --- but there may be a
threshold at which point it's not worth trying

[caitp (gmail), 2016-03-03 17:49:14]

On 2016/03/03 04:12:59, caitp wrote:
> Last patchset is a trial to see if some of the advantages of the descriptor
> array walking can still be used when elements are present, seems to work out
> okay: http://jsperf.com/lodash-values-vs-object-values/4 --- but there may be
a
> threshold at which point it's not worth trying

To expand on that last point, this version seems to win over lodash even when
the object contains a fairly large number of elements (either sparse or packed),
as demonstrated by http://jsperf.com/lodash-values-vs-object-values/5 ---
jsperf's cumulative results graph seems to be broken, but some screenshots sort
of demonstrate the difference the patchset makes

(Screenshots of runs at
https://www.dropbox.com/s/2kepsl5egwko14r/with-opt.png?dl=0 /
https://www.dropbox.com/s/24fp96qmcemq334/without-opt.png?dl=0).

It's not an enormous win, but it seems worth having.

[neis, 2016-03-04 09:37:49]

Description was changed from

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names, no elements
present), use the receiver's EnumCache rather than walking the object and
verifying enumerable status.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

to

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names, no elements
present), use the receiver's EnumCache rather than walking the object and
verifying enumerable status.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

[neis, 2016-03-04 09:37:49]

neis@chromium.org changed reviewers:
- neis@chromium.org

[Camillo Bruni, 2016-03-04 09:40:24]

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

[Camillo Bruni, 2016-03-04 09:40:25]

I'm fine with patch 4 and maybe postpone further optimizations to follow-up CLs.

https://codereview.chromium.org/1751643003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1751643003/diff/60001/src/objects.cc#newcode8851
src/objects.cc:8851: int length = map->NumberOfOwnDescriptors();
nit: rename to nof_own_descriptors

https://codereview.chromium.org/1751643003/diff/60001/src/objects.cc#newcode8860
src/objects.cc:8860: Handle<Object> prop_value;
Have you tried to be dirty an use a pointer here instead of a handle (+
DisallowHeapAllocation)?
(probably will only have minimal influence)

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8853: LookupIterator::OWN_SKIP_INTERCEPTOR);
I think you will only have the full benefits when implementing this directly in
ElementsAccessor in elements.cc.
If you implement it in elements.cc you won't have to get a LookupIterator (even
though that's most likely to be inlined here).

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8859: if (!it.IsEnumerable()) continue;
For instance the enumerability check would only be necessary for
DICTIONARY_ELEMENTS

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8866: isolate, prop_value, Object::GetProperty(&it),
Nothing<bool>());
Same here, you can only have a getter with DICTIONARY_ELEMENTS

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8873: if (!next_key->IsName()) {
element keys should always be Numbers.
DCHECK(next_key->IsNumber()) and
Uint32ToString(Number::cast(*next_key)->value()) might avoid some additional
indirections.

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

https://codereview.chromium.org/1751643003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1751643003/diff/60001/src/objects.cc#newcode8860
src/objects.cc:8860: Handle<Object> prop_value;
On 2016/03/04 09:40:25, cbruni wrote:
> Have you tried to be dirty an use a pointer here instead of a handle (+
> DisallowHeapAllocation)?
> (probably will only have minimal influence)

Doesn't the "if (get_entries)" block on line 8892 preclude
DisallowHeapAllocation? (I'm not sure if it would be safe to add an
`AllowHeapAllocation` in that block or not)

[caitp (gmail), 2016-03-04 16:34:33]

Thanks --- I've just gone ahead and removed the elements handling for now, but
it would be a pity if fast paths couldn't be taken just because of indexed
properties

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8853: LookupIterator::OWN_SKIP_INTERCEPTOR);
On 2016/03/04 09:40:25, cbruni wrote:
> I think you will only have the full benefits when implementing this directly
in
> ElementsAccessor in elements.cc.
> If you implement it in elements.cc you won't have to get a LookupIterator
(even
> though that's most likely to be inlined here).

Acknowledged.

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8859: if (!it.IsEnumerable()) continue;
On 2016/03/04 09:40:25, cbruni wrote:
> For instance the enumerability check would only be necessary for
> DICTIONARY_ELEMENTS

Acknowledged.

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8866: isolate, prop_value, Object::GetProperty(&it),
Nothing<bool>());
On 2016/03/04 09:40:25, cbruni wrote:
> Same here, you can only have a getter with DICTIONARY_ELEMENTS

Acknowledged.

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8873: if (!next_key->IsName()) {
On 2016/03/04 09:40:25, cbruni wrote:
> element keys should always be Numbers.
> DCHECK(next_key->IsNumber()) and
> Uint32ToString(Number::cast(*next_key)->value()) might avoid some additional
> indirections.

Dictionary gave me the impression that they could be represented in the array as
Strings, I guess not though.

Anyways, I think this whole routine can be tackled in a followup patch (I will
opt to land this patch where the fast path is not taken when elements are
present).

[Camillo Bruni, 2016-03-04 17:23:27]

lgtm

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1751643003/diff/100001/src/objects.cc#newcode...
src/objects.cc:8873: if (!next_key->IsName()) {
On 2016/03/04 at 16:34:33, caitp wrote:
> On 2016/03/04 09:40:25, cbruni wrote:
> > element keys should always be Numbers.
> > DCHECK(next_key->IsNumber()) and
> > Uint32ToString(Number::cast(*next_key)->value()) might avoid some additional
> > indirections.
> 
> Dictionary gave me the impression that they could be represented in the array
as Strings, I guess not though.
> 
> Anyways, I think this whole routine can be tackled in a followup patch (I will
opt to land this patch where the fast path is not taken when elements are
present).

Sure :).

DICTIONARY_ELEMENTS is used for elements only (indices), but uses a dictionary
to store the indices => value mapping. You currently get that if the indices are
out of smi, but in the precise double range, or if you define a non-simple
property with an index key (elements.cc tries to be your friend... but I agree
it's rather complex).

[caitp (gmail), 2016-03-04 19:25:11]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-04 19:25:11]

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/1751643003/#ps160001
(title: "fix tiny bug")

[commit-bot: I haz the power, 2016-03-04 19:25:21]

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

[commit-bot: I haz the power, 2016-03-04 19:25:22]

This CL has an open dependency (Issue 1746383003 Patch 120001). Please resolve
the dependency and try again.

[caitp (gmail), 2016-03-04 19:25:58]

On 2016/03/04 19:25:22, commit-bot: I haz the power wrote:
> This CL has an open dependency (Issue 1746383003 Patch 120001). Please resolve
> the dependency and try again.

Was curious what would happen in this case :)

[caitp (gmail), 2016-03-07 14:06:45]

Description was changed from

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names, no elements
present), use the receiver's EnumCache rather than walking the object and
verifying enumerable status.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

to

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

[caitp (gmail), 2016-03-07 14:07:06]

Description was changed from

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

to

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

[caitp (gmail), 2016-03-07 22:05:18]

Description was changed from

==========
[esnext] use enum cache when possible in Object.values() and entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

to

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

[caitp (gmail), 2016-03-07 22:07:14]

Ping Adam or Dan: can you sign off (or add another runtime person to sign off
for you)?

[adamk, 2016-03-07 22:21:58]

adamk@chromium.org changed reviewers:
+ verwaest@chromium.org
- littledan@chromium.org, neis@chromium.org

[adamk, 2016-03-07 22:21:59]

I'm going to punt to verwaest@, a runtime person who's also been doing a lot of
work here.

Also I see some nits from cbruni@ on patchset 4 that are still present in the
latest patchset...

[Toon Verwaest, 2016-03-07 22:39:34]

lgtm

https://codereview.chromium.org/1751643003/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1751643003/diff/160001/src/objects.cc#newcode...
src/objects.cc:8859: if (filter == ENUMERABLE_STRINGS && object->IsJSObject()) {
You don't need object->IsJSObject since that's already checked by
FastGetOwnValuesOr...

[caitp (gmail), 2016-03-07 23:19:02]

Description was changed from

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org,
neis@chromium.org
==========

to

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, verwaest@chromium.org, adamk@chromium.org
==========

[caitp (gmail), 2016-03-07 23:22:13]

On 2016/03/07 22:39:34, Toon Verwaest wrote:
> lgtm
> 
> https://codereview.chromium.org/1751643003/diff/160001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/1751643003/diff/160001/src/objects.cc#newcode...
> src/objects.cc:8859: if (filter == ENUMERABLE_STRINGS && object->IsJSObject())
{
> You don't need object->IsJSObject since that's already checked by
> FastGetOwnValuesOr...

Thanks --- I think that takes care of all of the nits.

[caitp (gmail), 2016-03-07 23:22:31]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-07 23:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1751643003/#ps200001
(title: "remove redundant IsJSObject() check")

[commit-bot: I haz the power, 2016-03-07 23:22:43]

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

[commit-bot: I haz the power, 2016-03-07 23:50:31]

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

[commit-bot: I haz the power, 2016-03-07 23:50:32]

Try jobs failed on following builders:
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/2496)
  v8_linux_dbg_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[caitp (gmail), 2016-03-07 23:54:39]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-07 23:54:40]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1751643003/#ps220001
(title: "make Handle<> typechecks happy with the IsJSObject() removed")

[commit-bot: I haz the power, 2016-03-07 23:54:51]

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

[commit-bot: I haz the power, 2016-03-07 23:56:46]

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

[commit-bot: I haz the power, 2016-03-07 23:56:48]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[caitp (gmail), 2016-03-08 00:09:24]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2016-03-08 00:09:25]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1751643003/#ps240001
(title: "re-fix above commit to actually be able to build >:|")

[commit-bot: I haz the power, 2016-03-08 00:09:30]

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

[commit-bot: I haz the power, 2016-03-08 00:29:39]

Description was changed from

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, verwaest@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, verwaest@chromium.org, adamk@chromium.org
==========

[commit-bot: I haz the power, 2016-03-08 00:29:40]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-08 00:30:57]

Description was changed from

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, verwaest@chromium.org, adamk@chromium.org
==========

to

==========
[esnext] use map instance_descriptors() when possible in Object.values/entries()

When possible (non-Proxy receiver, expecting only String-names), walk the
instance_descriptors() array rather than performing [[OwnPropertyKeys]]. If the
map changes during a call to an accessor property, resort to a slower property
lookup.

For now, the fast path is not taken if the object contains any element keys.

Offers a measurable improvement over the existing version, in select situations.

BUG=v8:4663
LOG=N
R=cbruni@chromium.org, verwaest@chromium.org, adamk@chromium.org

Committed: https://crrev.com/cee0dca2b9c52e83a6b1ab368662f7029b480d20
Cr-Commit-Position: refs/heads/master@{#34567}
==========

[commit-bot: I haz the power, 2016-03-08 00:30:59]

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