[stubs] KeyedStoreGeneric: inline dictionary property stores

[stubs] KeyedStoreGeneric: inline dictionary property stores

For dictionary-mode receivers, the KeyedStoreGeneric stub can store
properties directly in most cases. Doing so avoids the need to have
an entry in the stub cache for every map/property combination.

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

[Jakob Kummerow, 2016-11-21 17:02:57]

jkummerow@chromium.org changed reviewers:
+ ishell@chromium.org, rmcilroy@chromium.org

[Jakob Kummerow, 2016-11-21 17:02:58]

Igor: please take a look.

Ross: please take a look at src/interpreter/ (trivial changes).

https://codereview.chromium.org/2504403005/diff/40001/src/builtins/builtins-a...
File src/builtins/builtins-array.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/builtins/builtins-a...
src/builtins/builtins-array.cc:2569: assembler.StoreFixedArrayElement(elements,
0, index, SKIP_WRITE_BARRIER);
Drive-by cleanup.

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.h:368: int additional_offset = 0,
This mirrors LoadFixedArrayElement().

https://codereview.chromium.org/2504403005/diff/40001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stubs.cc#newco...
src/code-stubs.cc:2624: assembler->StoreFixedArrayElement(function_context,
Context::CLOSURE_INDEX,
More drive-by cleanup.

[rmcilroy, 2016-11-21 17:25:18]

interpreter/ LGTM with a suggestion (which would mean no changes were needed in
interpretr/ anyway).

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1338: int additional_offset,
Would it be worth reordering these so that you don't need to specify
additional_offset if you want to specify barrier_mode (it seems like code want's
to change barrier_mode more often than having an additional_offset != 0)?

[Igor Sheludko, 2016-11-22 12:05:08]

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1338: int additional_offset,
On 2016/11/21 17:25:18, rmcilroy wrote:
> Would it be worth reordering these so that you don't need to specify
> additional_offset if you want to specify barrier_mode (it seems like code
want's
> to change barrier_mode more often than having an additional_offset != 0)?

+1. And if you put additional parameter after WriteBarrierMode we can postpone
converting the latter to enum class.

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4140: DCHECK(mode == kFindInsertionIndex);
DCHECK_EQ

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4264: void CodeStubAssembler::Add(Node* dictionary,
Node* key, Node* value,
This implementation is not fully applicable for GlobalDictionary. Maybe define
Add() same way as you defined FindInsertionEntry().

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4281:
Smi::FromInt(PropertyDetails::DictionaryStorageField::kMax));
Can you drop Smi::FromInt?

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4303: if (Dictionary::kIsEnumerable) {
Shoudn't we also set details if the dictionary is not enumerable?

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.h:362: return StoreFixedArrayElement(object,
Int32Constant(index), value, 0,
While you are here: IntPtrOrSmiConstant(index, mode)

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.h:368: int additional_offset = 0,
On 2016/11/21 17:02:58, Jakob Kummerow wrote:
> This mirrors LoadFixedArrayElement().

Since WriteBarrierMode is defined as an enum but not as an enum class it could
be silently converted to int by the compiler if someone will forget to provide
additional_offset parameter:

  StoreFixedArrayElement(node1, node2, node3, SKIP_WRITE_BARRIER);

BTW, I had similar issues with LoadFixedArrayElement where the parameter mode
can be used as an additional offset.

To avoid such issues we should make WriteBarrierMode and ParameterMode enum
classes.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:493: GotoIf(IsSetWord32(details,
PropertyDetails::kAttributesReadOnlyMask),
Please add a comment that accessor properties never have READ_ONLY attribute.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:570: const int kNameToValueOffset =
(GlobalDictionary::kEntryValueIndex -
Idea for next CL: add helper functions for accessing value and details by entry.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:676:
GotoIf(Word32Equal(LoadInstanceType(accessor_pair),
You can compare maps instead of instance types.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:683:
GotoIf(Word32Equal(LoadMapInstanceType(setter_map),
Same here.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:700: DCHECK(language_mode == SLOPPY);
DCHECK_EQ

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:715: DCHECK(language_mode == SLOPPY);
DCHECK_EQ

[Jakob Kummerow, 2016-11-22 13:47:28]

Patchset #4 (id:60001) has been deleted

[Jakob Kummerow, 2016-11-22 13:47:57]

Thanks for the reviews. Comments addressed.

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1338: int additional_offset,
On 2016/11/22 12:05:07, Igor Sheludko wrote:
> On 2016/11/21 17:25:18, rmcilroy wrote:
> > Would it be worth reordering these so that you don't need to specify
> > additional_offset if you want to specify barrier_mode (it seems like code
> want's
> > to change barrier_mode more often than having an additional_offset != 0)?
> 
> +1. And if you put additional parameter after WriteBarrierMode we can postpone
> converting the latter to enum class.

Done.

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4140: DCHECK(mode == kFindInsertionIndex);
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> DCHECK_EQ

Done. (FWIW, I disagree with the preference -- I think cognitive load for the
reader is lower if the DCHECK reads like "if (mode == kFindInsertionIndex)", but
whatever.)

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4264: void CodeStubAssembler::Add(Node* dictionary,
Node* key, Node* value,
On 2016/11/22 12:05:07, Igor Sheludko wrote:
> This implementation is not fully applicable for GlobalDictionary. Maybe define
> Add() same way as you defined FindInsertionEntry().

Done. (Pulled out the second half as "InsertEntry<Dictionary>".)

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4281:
Smi::FromInt(PropertyDetails::DictionaryStorageField::kMax));
On 2016/11/22 12:05:07, Igor Sheludko wrote:
> Can you drop Smi::FromInt?

Done.

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:4303: if (Dictionary::kIsEnumerable) {
On 2016/11/22 12:05:07, Igor Sheludko wrote:
> Shoudn't we also set details if the dictionary is not enumerable?

Oops, good point. Done.

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.h:362: return StoreFixedArrayElement(object,
Int32Constant(index), value, 0,
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> While you are here: IntPtrOrSmiConstant(index, mode)

Done, kind of. |parameter_mode| is pretty meaningless here, no caller cares
anyway, and if they did override it, it would be a bug... I've used
IntPtrConstant and dropped the parameter. OK?

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.h:368: int additional_offset = 0,
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> On 2016/11/21 17:02:58, Jakob Kummerow wrote:
> > This mirrors LoadFixedArrayElement().
> 
> Since WriteBarrierMode is defined as an enum but not as an enum class it could
> be silently converted to int by the compiler if someone will forget to provide
> additional_offset parameter:
> 
>   StoreFixedArrayElement(node1, node2, node3, SKIP_WRITE_BARRIER);
> 
> BTW, I had similar issues with LoadFixedArrayElement where the parameter mode
> can be used as an additional offset.
> 
> To avoid such issues we should make WriteBarrierMode and ParameterMode enum
> classes.

Yes -> followup.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:493: GotoIf(IsSetWord32(details,
PropertyDetails::kAttributesReadOnlyMask),
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> Please add a comment that accessor properties never have READ_ONLY attribute.

Done.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:570: const int kNameToValueOffset =
(GlobalDictionary::kEntryValueIndex -
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> Idea for next CL: add helper functions for accessing value and details by
entry.

Acknowledged. Good idea. An alternative I've considered is to move the constants
into the respective Dictionary class, so here we could simply say
"GlobalDictionary::kNameToValueOffset".

Added a TODO for now.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:676:
GotoIf(Word32Equal(LoadInstanceType(accessor_pair),
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> You can compare maps instead of instance types.

Done. (I wonder which is faster...)

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:683:
GotoIf(Word32Equal(LoadMapInstanceType(setter_map),
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> Same here.

Done.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:700: DCHECK(language_mode == SLOPPY);
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:715: DCHECK(language_mode == SLOPPY);
On 2016/11/22 12:05:08, Igor Sheludko wrote:
> DCHECK_EQ

Done.

[Igor Sheludko, 2016-11-22 14:07:31]

Lgtm with a couple of nits:

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2504403005/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.h:362: return StoreFixedArrayElement(object,
Int32Constant(index), value, 0,
On 2016/11/22 13:47:57, Jakob Kummerow wrote:
> On 2016/11/22 12:05:08, Igor Sheludko wrote:
> > While you are here: IntPtrOrSmiConstant(index, mode)
> 
> Done, kind of. |parameter_mode| is pretty meaningless here, no caller cares
> anyway, and if they did override it, it would be a bug... I've used
> IntPtrConstant and dropped the parameter. OK?

Indeed :)

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2504403005/diff/40001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:676:
GotoIf(Word32Equal(LoadInstanceType(accessor_pair),
On 2016/11/22 13:47:57, Jakob Kummerow wrote:
> On 2016/11/22 12:05:08, Igor Sheludko wrote:
> > You can compare maps instead of instance types.
> 
> Done. (I wonder which is faster...)

I heard rumors that CSA can replace LoadRoot() with a constant if it makes sense
for particular platform and root index. So I think it should be faster.

https://codereview.chromium.org/2504403005/diff/80001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2504403005/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:680: LoadRoot(Heap::kAccessorPairMapRootIndex)),
Consider populating HEAP_CONSTANT_LIST to use
  GotoIf(IsAccessorPairMap(LoadMap(accessor_pair)), ...);

Probably there will be more places where we will need this kind of check.

https://codereview.chromium.org/2504403005/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:687:
LoadRoot(Heap::kFunctionTemplateInfoMapRootIndex)),
Same here.

[Jakob Kummerow, 2016-11-22 14:20:49]

Thanks, landing.

https://codereview.chromium.org/2504403005/diff/80001/src/ic/keyed-store-gene...
File src/ic/keyed-store-generic.cc (right):

https://codereview.chromium.org/2504403005/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:680: LoadRoot(Heap::kAccessorPairMapRootIndex)),
On 2016/11/22 14:07:31, Igor Sheludko wrote:
> Consider populating HEAP_CONSTANT_LIST to use
>   GotoIf(IsAccessorPairMap(LoadMap(accessor_pair)), ...);
> 
> Probably there will be more places where we will need this kind of check.

Done.

https://codereview.chromium.org/2504403005/diff/80001/src/ic/keyed-store-gene...
src/ic/keyed-store-generic.cc:687:
LoadRoot(Heap::kFunctionTemplateInfoMapRootIndex)),
On 2016/11/22 14:07:31, Igor Sheludko wrote:
> Same here.

Done.

[Jakob Kummerow, 2016-11-22 14:21:00]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-11-22 14:21:00]

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

[commit-bot: I haz the power, 2016-11-22 14:21:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 14:51:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479824460032600,
"parent_rev": "3600848f211af1a8567afcd2188c7b00efbaf711", "commit_rev":
"30a8712850f0deb221ae07c22b435341911af26f"}

[commit-bot: I haz the power, 2016-11-22 14:51:35]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-22 14:52:10]

Description was changed from

==========
[stubs] KeyedStoreGeneric: inline dictionary property stores

For dictionary-mode receivers, the KeyedStoreGeneric stub can store
properties directly in most cases. Doing so avoids the need to have
an entry in the stub cache for every map/property combination.
==========

to

==========
[stubs] KeyedStoreGeneric: inline dictionary property stores

For dictionary-mode receivers, the KeyedStoreGeneric stub can store
properties directly in most cases. Doing so avoids the need to have
an entry in the stub cache for every map/property combination.

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

[commit-bot: I haz the power, 2016-11-22 14:52:12]

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

[Michael Hablich, 2016-11-23 08:19:30]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/2528583002/ by hablich@chromium.org.

The reason for reverting is: Blocks roll:
https://codereview.chromium.org/2526573002/.