[runtime] Object.create(null) creates a slow object

[runtime] Object.create(null) creates a slow object

Object.create(null) is most likely to be used for dictionary-like objects.
Hence it would be beneficial to directly create a slow-mode object and avoid
additional overhead later-on.

BUG=

Committed: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
Cr-Commit-Position: refs/heads/master@{#40551}

[Camillo Bruni, 2016-10-21 07:10:46]

Description was changed from

==========
0# Enter a description of the change.
[runtime] Object.create(null) creates a slow object.

Object.create(null) is most likely to be used for dictionary-like objects.
Hence it would be beneficial to directly create a slow-mode object and avoid
additional overhead later-on.

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

to

==========
[runtime] Object.create(null) creates a slow object

Object.create(null) is most likely to be used for dictionary-like objects.
Hence it would be beneficial to directly create a slow-mode object and avoid
additional overhead later-on.

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

[Camillo Bruni, 2016-10-21 07:12:08]

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

[Camillo Bruni, 2016-10-21 07:12:08]

PTAL
For now I did not templetize the dictionary instantiation as there is only one
user.
I could split AllocateNameDictionary up and make sure we have separate helper
for each class in the NameDictionary hierarchy that adds fields.

[Igor Sheludko, 2016-10-21 07:48:28]

First round of comments.

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

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:130: void
CodeStubAssembler::BranchIfWordIsPowerOfTwo(Node* value, Label* if_true,
I think this can be written as predicate IsPowerOfTwoWord() if you use Select
instructions...

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1682: BranchIfWordIsPowerOfTwo(capacity, &ok,
&not_ok);
... and them this will just be CSA_ASSERT(IsPowerOfTwo(capacity)).

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1689: Node* store_size =
Please follow USE_DEFAULT_MINIMUM_CAPACITY way of calculating size.

USE_CUSTOM_MINIMUM_CAPACITY does not actually work because whenever you add
anything to the dictionary the call to EnsureCapacity() will reallocate it to at
least USE_DEFAULT_MINIMUM_CAPACITY.

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1700: Node* zero = SmiConstant(0);
Smi::kZero

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

https://codereview.chromium.org/2430273007/diff/20001/src/objects.cc#newcode1...
src/objects.cc:16773: DCHECK_IMPLIES(capacity_option ==
USE_CUSTOM_MINIMUM_CAPACITY,
FYI: USE_CUSTOM_MINIMUM_CAPACITY does not actually work because whenever you add
anything to the dictionary EnsureCapacity() will reallocate it to at least
USE_DEFAULT_MINIMUM_CAPACITY.

https://codereview.chromium.org/2430273007/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2430273007/diff/20001/src/objects.h#newcode3485
src/objects.h:3485: static const int kNumberOfElementsOffset = kHeaderSize;
I think it is better to use OffsetOfElementAt() to calculate offsets instead of
defining these offset constants.

https://codereview.chromium.org/2430273007/diff/20001/src/objects.h#newcode3849
src/objects.h:3849: static const int kMaxNumberKeyOffset =
DerivedHashTable::kPrefixStartOffset;
Same here.

[Camillo Bruni, 2016-10-21 13:37:16]

PTAl again

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

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:130: void
CodeStubAssembler::BranchIfWordIsPowerOfTwo(Node* value, Label* if_true,
On 2016/10/21 at 07:48:27, Igor Sheludko wrote:
> I think this can be written as predicate IsPowerOfTwoWord() if you use Select
instructions...

Ahhh that's what I've been looking for yesterday! thanks!

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1682: BranchIfWordIsPowerOfTwo(capacity, &ok,
&not_ok);
On 2016/10/21 at 07:48:27, Igor Sheludko wrote:
> ... and them this will just be CSA_ASSERT(IsPowerOfTwo(capacity)).

done.

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1689: Node* store_size =
On 2016/10/21 at 07:48:27, Igor Sheludko wrote:
> Please follow USE_DEFAULT_MINIMUM_CAPACITY way of calculating size.
> 
> USE_CUSTOM_MINIMUM_CAPACITY does not actually work because whenever you add
anything to the dictionary the call to EnsureCapacity() will reallocate it to at
least USE_DEFAULT_MINIMUM_CAPACITY.

Implemented HashTableComputeCapacity.

https://codereview.chromium.org/2430273007/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1700: Node* zero = SmiConstant(0);
On 2016/10/21 at 07:48:27, Igor Sheludko wrote:
> Smi::kZero

I added the SmiConstant(int) given the frequent use of
SmiConstant(Smi::FromInt(i)) to add some convenience.

https://codereview.chromium.org/2430273007/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2430273007/diff/20001/src/objects.h#newcode3485
src/objects.h:3485: static const int kNumberOfElementsOffset = kHeaderSize;
On 2016/10/21 at 07:48:27, Igor Sheludko wrote:
> I think it is better to use OffsetOfElementAt() to calculate offsets instead
of defining these offset constants.

done.

https://codereview.chromium.org/2430273007/diff/20001/src/objects.h#newcode3849
src/objects.h:3849: static const int kMaxNumberKeyOffset =
DerivedHashTable::kPrefixStartOffset;
On 2016/10/21 at 07:48:27, Igor Sheludko wrote:
> Same here.

done.

https://codereview.chromium.org/2430273007/diff/40001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2430273007/diff/40001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:519:
a->AllocateNameDictionary(NameDictionary::kInitialCapacity));
How can I make sure the GC updates the properties pointer correctly if we
trigger a GC later-on when allocating the JSObject in AllocateJSObjectFromMap?

https://codereview.chromium.org/2430273007/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2430273007/diff/40001/src/objects.h#newcode3582
src/objects.h:3582: (FixedArray::kMaxLength - kElementsStartIndex) / kEntrySize;
Note: I change kElementsStartOffset to kElementsStartIndex from the original
code.

[Igor Sheludko, 2016-10-21 15:01:10]

Almost there...

https://codereview.chromium.org/2430273007/diff/40001/src/builtins/builtins-o...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2430273007/diff/40001/src/builtins/builtins-o...
src/builtins/builtins-object.cc:519:
a->AllocateNameDictionary(NameDictionary::kInitialCapacity));
On 2016/10/21 13:37:15, Camillo Bruni wrote:
> How can I make sure the GC updates the properties pointer correctly if we
> trigger a GC later-on when allocating the JSObject in AllocateJSObjectFromMap?

You marked the variable as kTagged, so the compiler will make GC know about yet
another value to update.

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

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:132: value = IntPtrSubWithOverflow(value,
IntPtrConstant(1));
You should use normal IntPtrSub() here since you don't care if the overflow
happened or not.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:133: for (int i = 1; i <= 16; i *= 2) {
This implementation is actually for word32. I don't know how to express this
fact in the name :( It's already quite long. Maybe s/value/word32/ and a comment
will help...

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:143: WordAnd(value, IntPtrSubWithOverflow(value,
IntPtrConstant(1)))),
Same here.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1128: return
Word32NotEqual(BitFieldDecode<Map::DictionaryMap>(bit_field3),
  return IsSetWord32<Map::DictionaryMap>(bit_field3);

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1704: StoreObjectFieldRoot(result,
Internals::kHeapObjectMapOffset,
Why not FixedArray::kMapOffset to be consistent?

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1727:
IntPtrConstant(NameDictionary::kElementsStartOffset - kHeapObjectTag));
(NameDictionay::OffsetOfElementAt(NameDictionary::kElementsStartIndex) -
kHeapObjectTag)
 or something...

https://codereview.chromium.org/2430273007/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2430273007/diff/40001/src/objects.h#newcode3493
src/objects.h:3493: static const int kPrefixStartOffset = kCapacityOffset +
kPointerSize;
I think you can remove these *Offset constants.

[Camillo Bruni, 2016-10-21 16:27:32]

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

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:133: for (int i = 1; i <= 16; i *= 2) {
On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> This implementation is actually for word32. I don't know how to express this
fact in the name :( It's already quite long. Maybe s/value/word32/ and a comment
will help...

The CSA_ASSERT is there for this. Will add a proper comment though.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:143: WordAnd(value, IntPtrSubWithOverflow(value,
IntPtrConstant(1)))),
On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> Same here.

If I do IntPtrSub the whole second part of the select instruction disappears.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1727:
IntPtrConstant(NameDictionary::kElementsStartOffset - kHeapObjectTag));
On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> (NameDictionay::OffsetOfElementAt(NameDictionary::kElementsStartIndex) -
kHeapObjectTag)
>  or something...

oh, somehow missed that :)

https://codereview.chromium.org/2430273007/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2430273007/diff/40001/src/objects.h#newcode3493
src/objects.h:3493: static const int kPrefixStartOffset = kCapacityOffset +
kPointerSize;
On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> I think you can remove these *Offset constants.

thought I did ;)

[Camillo Bruni, 2016-10-24 09:53:02]

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

[Camillo Bruni, 2016-10-24 09:53:07]

PTAL again

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

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:132: value = IntPtrSubWithOverflow(value,
IntPtrConstant(1));
On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> You should use normal IntPtrSub() here since you don't care if the overflow
happened or not.

done.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:133: for (int i = 1; i <= 16; i *= 2) {
On 2016/10/21 at 16:27:32, Camillo Bruni wrote:
> On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> > This implementation is actually for word32. I don't know how to express this
fact in the name :( It's already quite long. Maybe s/value/word32/ and a comment
will help...
> 
> The CSA_ASSERT is there for this. Will add a proper comment though.

done.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:143: WordAnd(value, IntPtrSubWithOverflow(value,
IntPtrConstant(1)))),
On 2016/10/21 at 16:27:32, Camillo Bruni wrote:
> On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> > Same here.
> 
> If I do IntPtrSub the whole second part of the select instruction disappears.

done.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1128: return
Word32NotEqual(BitFieldDecode<Map::DictionaryMap>(bit_field3),
On 2016/10/21 at 15:01:09, Igor Sheludko wrote:
> return IsSetWord32<Map::DictionaryMap>(bit_field3);

done.

https://codereview.chromium.org/2430273007/diff/40001/src/code-stub-assembler...
src/code-stub-assembler.cc:1704: StoreObjectFieldRoot(result,
Internals::kHeapObjectMapOffset,
On 2016/10/21 at 15:01:10, Igor Sheludko wrote:
> Why not FixedArray::kMapOffset to be consistent?

yup, done.

[commit-bot: I haz the power, 2016-10-24 09:53:08]

Dry run: 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-10-24 10:07:49]

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

[commit-bot: I haz the power, 2016-10-24 10:07:50]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[Igor Sheludko, 2016-10-24 12:00:55]

lgtm with a nit, modulo redness.

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

https://codereview.chromium.org/2430273007/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:130: Node*
CodeStubAssembler::IntPtrRoundUpToPowerOfTwo32(Node* word32) {
I'm sorry for going back and forth. With word32 it's even more confusing. Please
rename it back to value and add a comment that we expect a word-size argument
here containing only a 32-bit value.

[Camillo Bruni, 2016-10-24 12:50:45]

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

[Camillo Bruni, 2016-10-24 12:50:45]

epertoso@ PTAL src/compiler/*

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

https://codereview.chromium.org/2430273007/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.cc:130: Node*
CodeStubAssembler::IntPtrRoundUpToPowerOfTwo32(Node* word32) {
On 2016/10/24 at 12:00:55, Igor Sheludko wrote:
> I'm sorry for going back and forth. With word32 it's even more confusing.
Please rename it back to value and add a comment that we expect a word-size
argument here containing only a 32-bit value.

no problem :)

[epertoso, 2016-10-24 13:01:45]

lgtm for compiler/

[Camillo Bruni, 2016-10-24 13:45:46]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-10-24 13:45:47]

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/2430273007/#ps80001
(title: "renaming variables and fixing cctest")

[commit-bot: I haz the power, 2016-10-24 13:45:51]

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-10-24 14:01:00]

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

[commit-bot: I haz the power, 2016-10-24 14:01:01]

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

[Camillo Bruni, 2016-10-24 20:53:08]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-10-24 20:53:09]

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

[commit-bot: I haz the power, 2016-10-24 20:53:14]

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-10-24 21:49:42]

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

[commit-bot: I haz the power, 2016-10-24 21:49:43]

Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15096)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[Camillo Bruni, 2016-10-25 08:01:30]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-10-25 08:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
epertoso@chromium.org
Link to the patchset: https://codereview.chromium.org/2430273007/#ps120001
(title: "fix GC mole issue")

[commit-bot: I haz the power, 2016-10-25 08:01:43]

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-10-25 08:30:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 22:12:03]

Description was changed from

==========
[runtime] Object.create(null) creates a slow object

Object.create(null) is most likely to be used for dictionary-like objects.
Hence it would be beneficial to directly create a slow-mode object and avoid
additional overhead later-on.

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

to

==========
[runtime] Object.create(null) creates a slow object

Object.create(null) is most likely to be used for dictionary-like objects.
Hence it would be beneficial to directly create a slow-mode object and avoid
additional overhead later-on.

BUG=

Committed: https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
Cr-Commit-Position: refs/heads/master@{#40551}
==========

[commit-bot: I haz the power, 2016-11-17 22:12:04]

Patchset 7 (id:??) landed as
https://crrev.com/532c16eca071df3ec8eed394dcebb932ef584ee6
Cr-Commit-Position: refs/heads/master@{#40551}