Ensure slow properties for simple {__proto__:null} literals.

[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
- The EmitFastCloneShallowObject builtin now supports cloning slow
  object boilerplates. 
- Unified behavior to find the matching Map and instantiating it for 
  Object.create(null) and literals with a null prototype.
- Cleanup of literal type parameter of CompileTimeValue, now in sync with 
  ObjectLiteral flags.

Review-Url: https://codereview.chromium.org/2445333002
Cr-Commit-Position: refs/heads/master@{#44941}
Committed: https://chromium.googlesource.com/v8/v8/+/3f73fecb132457ba66b1cfbd04a025b33ffef3b7

[Camillo Bruni, 2016-10-25 09:57:35]

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

[Camillo Bruni, 2016-10-25 09:57:36]

PTAL
First attempt: I hope this is not too hacky.

[Toon Verwaest, 2016-10-25 11:10:10]

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

[Toon Verwaest, 2016-10-25 11:10:10]

What about %HaveSameMap({__proto__:null}, Object.create(null)) ?

[adamk, 2016-10-25 11:14:24]

https://codereview.chromium.org/2445333002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2445333002/diff/1/src/ast/ast.h#newcode1425
src/ast/ast.h:1425: if (is_simple() || properties_count() != 0) return false;
I don't think properties_count() gives you the right answer...why not just check
for properties()->length() == 1?

https://codereview.chromium.org/2445333002/diff/1/test/mjsunit/object-literal.js
File test/mjsunit/object-literal.js (right):

https://codereview.chromium.org/2445333002/diff/1/test/mjsunit/object-literal...
test/mjsunit/object-literal.js:269: assertFalse(%HasFastProperties(obj));
You'll need --allow-natives-syntax in Flags

https://codereview.chromium.org/2445333002/diff/1/test/mjsunit/object-literal...
test/mjsunit/object-literal.js:273: var obj2 = {__proto__:null, a:1, b:2};
Please add a test with computed properties and non-compile-time-values, e.g.

obj3 = {__proto__: null, ["a"]: 1}

obj4 = {__proto__: null, a: obj3}

[Camillo Bruni, 2016-10-25 16:26:22]

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

[commit-bot: I haz the power, 2016-10-25 16:26:25]

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-25 16:39:57]

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

[commit-bot: I haz the power, 2016-10-25 16:39:58]

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

[Camillo Bruni, 2016-10-25 16:43:12]

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

[commit-bot: I haz the power, 2016-10-25 16:43:17]

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-25 16:54:29]

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

[commit-bot: I haz the power, 2016-10-25 16:54:30]

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...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Camillo Bruni, 2016-10-26 13:19:13]

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

[commit-bot: I haz the power, 2016-10-26 13:19:25]

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-26 13:34:39]

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

[commit-bot: I haz the power, 2016-10-26 13:34:40]

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...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Camillo Bruni, 2016-10-27 09:24:39]

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

[commit-bot: I haz the power, 2016-10-27 09:24:48]

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-27 09:41:44]

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

[commit-bot: I haz the power, 2016-10-27 09:41:45]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15335)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11583)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Camillo Bruni, 2017-03-13 15:50:30]

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

[commit-bot: I haz the power, 2017-03-13 15:50:32]

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, 2017-03-13 16:04:58]

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

[commit-bot: I haz the power, 2017-03-13 16:04:59]

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

[Camillo Bruni, 2017-03-13 16:18:16]

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

[commit-bot: I haz the power, 2017-03-13 16:18:19]

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, 2017-03-13 16:45:42]

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

[commit-bot: I haz the power, 2017-03-13 16:45:43]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-03-13 17:21:44]

PTAL again.
I revived this and fixed missing parts.

https://codereview.chromium.org/2445333002/diff/1/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2445333002/diff/1/src/ast/ast.h#newcode1425
src/ast/ast.h:1425: if (is_simple() || properties_count() != 0) return false;
On 2016/10/25 at 11:14:24, adamk wrote:
> I don't think properties_count() gives you the right answer...why not just
check for properties()->length() == 1?

That's probably easier, right.

https://codereview.chromium.org/2445333002/diff/1/test/mjsunit/object-literal.js
File test/mjsunit/object-literal.js (right):

https://codereview.chromium.org/2445333002/diff/1/test/mjsunit/object-literal...
test/mjsunit/object-literal.js:269: assertFalse(%HasFastProperties(obj));
On 2016/10/25 at 11:14:24, adamk wrote:
> You'll need --allow-natives-syntax in Flags

arg, bitten by my own smart aliases that add these flags automatically :P

https://codereview.chromium.org/2445333002/diff/1/test/mjsunit/object-literal...
test/mjsunit/object-literal.js:273: var obj2 = {__proto__:null, a:1, b:2};
On 2016/10/25 at 11:14:24, adamk wrote:
> Please add a test with computed properties and non-compile-time-values, e.g.
> 
> obj3 = {__proto__: null, ["a"]: 1}
> 
> obj4 = {__proto__: null, a: obj3}

done.

[Camillo Bruni, 2017-03-13 17:22:10]

Description was changed from

==========
Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).

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

to

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
==========

[Camillo Bruni, 2017-03-13 17:25:49]

Description was changed from

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
==========

to

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
With this CL the EmitFastCloneShallowObject builtin supports cloning slow object
boilerplates. Furthermore this CL unifies behavior to find the matching Map and
instantiating it for Object.create(null) and literals with a null prototype.
==========

[adamk, 2017-03-13 18:10:44]

AST stuff looks good to me, but Toon will be a better reviewer for the rest.

https://codereview.chromium.org/2445333002/diff/160001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2445333002/diff/160001/src/ast/ast.cc#newcode552
src/ast/ast.cc:552: // __proto__:null at the first position has no side-effects
and is set
Why do you say "first position"? The placement of the null proto isn't
important, I don't think.

https://codereview.chromium.org/2445333002/diff/160001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2445333002/diff/160001/src/ast/ast.h#newcode1358
src/ast/ast.h:1358: return value()->IsNullLiteral();
Nit:

return IsPrototype() && value()->IsNullLiteral()

seems just as clear to me.

[Camillo Bruni, 2017-03-14 10:07:07]

Description was changed from

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
With this CL the EmitFastCloneShallowObject builtin supports cloning slow object
boilerplates. Furthermore this CL unifies behavior to find the matching Map and
instantiating it for Object.create(null) and literals with a null prototype.
==========

to

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
With this CL the EmitFastCloneShallowObject builtin supports cloning slow
object boilerplates. Furthermore this CL unifies behavior to find the matching
Map and instantiating it for Object.create(null) and literals with a null
prototype.
==========

[Toon Verwaest, 2017-03-14 13:27:48]

https://codereview.chromium.org/2445333002/diff/180001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:608: *slow_object_with_null_prototype_map);
This already exists?

https://codereview.chromium.org/2445333002/diff/180001/src/builtins/builtins-...
File src/builtins/builtins-constructor.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/builtins/builtins-...
src/builtins/builtins-constructor.cc:743: SKIP_WRITE_BARRIER, SMI_PARAMETERS);
With black allocation I'm not sure this is safe if the array ends up in old
space. AllocateNameDictionary isn't explicitly allocating in new space, and
could be LOS at least.

https://codereview.chromium.org/2445333002/diff/180001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/factory.cc#newcode...
src/factory.cc:2614: Handle<Map>
Factory::ObjectLiteralMapFromCache(Handle<Context> context,
Isn't this the native context?

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
File src/runtime/runtime-literals.cc (left):

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:57: !is_result_from_cache &&
boilerplate->HasFastProperties();
This code is a little weird. I think we can even
CHECK(boilerplate->HasFastProperties()); at this point. And
!is_result_from_cache is based on 128 max tracking limit in
ComputeObjectLiteralMap. Why don't we just return a default
normalized-object-map instead and avoid this check?

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:60: // TODO(verwaest): We might not want to ever
normalize here.
Why are you removing this?

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:98: // constant function properties.
This comment doesn't make sense anymore afaict. We don't account for functions
at all.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:99: if (should_transform) {
I'm not actually sure whether it makes sense to make these objects fast again by
now. If they end up as .prototype they'll be made fast there anyway. Probably
should investigate in another CL though.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
File src/runtime/runtime-literals.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:180:
Handle<BoilerplateDescription>::cast(elements);
If we make sure that props[0] contains __proto__:null if it's there, we can also
support nested literals?

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-ob...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:249: if (!map->is_dictionary_map()) {
swap branches and drop ! ;)

[Camillo Bruni, 2017-03-16 14:38:22]

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

[commit-bot: I haz the power, 2017-03-16 14:38:29]

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, 2017-03-16 14:39:57]

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

[commit-bot: I haz the power, 2017-03-16 14:39:58]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  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...)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/36867)

[Camillo Bruni, 2017-03-16 14:42:50]

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

[commit-bot: I haz the power, 2017-03-16 14:42:58]

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, 2017-03-16 15:01:30]

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

[commit-bot: I haz the power, 2017-03-16 15:01:31]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/24370)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Camillo Bruni, 2017-03-17 16:08:00]

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

[commit-bot: I haz the power, 2017-03-17 16:08:11]

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, 2017-03-17 16:16:19]

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

[commit-bot: I haz the power, 2017-03-17 16:16:20]

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

[Camillo Bruni, 2017-03-17 16:17:40]

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

[commit-bot: I haz the power, 2017-03-17 16:17:47]

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

[Camillo Bruni, 2017-03-17 16:40:55]

PTAL again

https://codereview.chromium.org/2445333002/diff/160001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2445333002/diff/160001/src/ast/ast.h#newcode1358
src/ast/ast.h:1358: return value()->IsNullLiteral();
On 2017/03/13 at 18:10:44, adamk wrote:
> Nit:
> 
> return IsPrototype() && value()->IsNullLiteral()
> 
> seems just as clear to me.

uhm, right, used to be more complex ;)

https://codereview.chromium.org/2445333002/diff/180001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:608: *slow_object_with_null_prototype_map);
On 2017/03/14 at 13:27:47, Toon Verwaest wrote:
> This already exists?

just moved to here, so it's available earlier in the bootstrapping for the
unscopable Array properties.

https://codereview.chromium.org/2445333002/diff/180001/src/builtins/builtins-...
File src/builtins/builtins-constructor.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/builtins/builtins-...
src/builtins/builtins-constructor.cc:743: SKIP_WRITE_BARRIER, SMI_PARAMETERS);
On 2017/03/14 at 13:27:47, Toon Verwaest wrote:
> With black allocation I'm not sure this is safe if the array ends up in old
space. AllocateNameDictionary isn't explicitly allocating in new space, and
could be LOS at least.

Adding separate helper to make the contract clearer, calling to the runtime for
now, probably a very infrequent case, having large-object properties.

https://codereview.chromium.org/2445333002/diff/180001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/factory.cc#newcode...
src/factory.cc:2614: Handle<Map>
Factory::ObjectLiteralMapFromCache(Handle<Context> context,
On 2017/03/14 at 13:27:47, Toon Verwaest wrote:
> Isn't this the native context?

yup, changed.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
File src/runtime/runtime-literals.cc (left):

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:57: !is_result_from_cache &&
boilerplate->HasFastProperties();
On 2017/03/14 at 13:27:47, Toon Verwaest wrote:
> This code is a little weird. I think we can even
CHECK(boilerplate->HasFastProperties()); at this point. And
!is_result_from_cache is based on 128 max tracking limit in
ComputeObjectLiteralMap. Why don't we just return a default
normalized-object-map instead and avoid this check?

added separate initial slow map for this case.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:60: // TODO(verwaest): We might not want to ever
normalize here.
On 2017/03/14 at 13:27:48, Toon Verwaest wrote:
> Why are you removing this?

readded.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:98: // constant function properties.
On 2017/03/14 at 13:27:47, Toon Verwaest wrote:
> This comment doesn't make sense anymore afaict. We don't account for functions
at all.

removed.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:99: if (should_transform) {
On 2017/03/14 at 13:27:48, Toon Verwaest wrote:
> I'm not actually sure whether it makes sense to make these objects fast again
by now. If they end up as .prototype they'll be made fast there anyway. Probably
should investigate in another CL though.

added TODO

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
File src/runtime/runtime-literals.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:180:
Handle<BoilerplateDescription>::cast(elements);
On 2017/03/14 at 13:27:48, Toon Verwaest wrote:
> If we make sure that props[0] contains __proto__:null if it's there, we can
also support nested literals?

ack.

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-ob...
File src/runtime/runtime-object.cc (right):

https://codereview.chromium.org/2445333002/diff/180001/src/runtime/runtime-ob...
src/runtime/runtime-object.cc:249: if (!map->is_dictionary_map()) {
On 2017/03/14 at 13:27:48, Toon Verwaest wrote:
> swap branches and drop ! ;)

done.

[commit-bot: I haz the power, 2017-03-17 16:43:28]

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

[commit-bot: I haz the power, 2017-03-17 16:43:29]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2017-03-20 14:47:11]

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc#newcode555
src/ast/ast.cc:555: if (!IsBoilerplateProperty(property)) {
This is actually just property->IsPrototype(). The != NULL check above can't hit
I think; and the test suite seems to agree.

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc#newcode567
src/ast/ast.cc:567: if (position > boilerplate_properties_) {
What about simply doing a loop:

if (position == boilerplate_properties_) {
  // Check if any of the remaining properties is __proto__:null.
  if (!has_null_prototype()) {
    for (; i < properties->length(); i++) {
      if (properties()->at(i)->IsNullPrototype()) {
        set_has_null_prototype(true);
        break;
      }
    }
  }
  break;
}

You could also have another flag "has_seen_prototype" in case the
IsBoilerplateProperty above hits; as an alternative to has_null_prototype().

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc#newcode618
src/ast/ast.cc:618: bit_field_ = HasElementsField::update(bit_field_, elements >
0);
Leftover code?

https://codereview.chromium.org/2445333002/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-constructor-gen.cc (right):

https://codereview.chromium.org/2445333002/diff/340001/src/builtins/builtins-...
src/builtins/builtins-constructor-gen.cc:679: properties.value());
Isn't this field overwritten by the initialization loop below, effectively
sharing the "empty dictionary" between all boilerplate copies?

[Camillo Bruni, 2017-03-27 14:49:44]

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

[commit-bot: I haz the power, 2017-03-27 14:49:51]

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, 2017-03-27 15:15:55]

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

[commit-bot: I haz the power, 2017-03-27 15:15:57]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-04-03 09:51:11]

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

[commit-bot: I haz the power, 2017-04-03 09:51:13]

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, 2017-04-03 10:16:27]

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

[commit-bot: I haz the power, 2017-04-03 10:16:28]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-04-19 09:10:21]

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

[commit-bot: I haz the power, 2017-04-19 09:10:26]

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, 2017-04-19 09:40:05]

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

[commit-bot: I haz the power, 2017-04-19 09:40:06]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-04-26 11:54:14]

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

[commit-bot: I haz the power, 2017-04-26 11:54:22]

Dry run: CQ is trying da patch.

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

[Camillo Bruni, 2017-04-26 11:54:46]

PTAL again

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc#newcode555
src/ast/ast.cc:555: if (!IsBoilerplateProperty(property)) {
On 2017/03/20 at 14:47:11, Toon Verwaest wrote:
> This is actually just property->IsPrototype(). The != NULL check above can't
hit I think; and the test suite seems to agree.

done.

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc#newcode567
src/ast/ast.cc:567: if (position > boilerplate_properties_) {
On 2017/03/20 at 14:47:11, Toon Verwaest wrote:
> What about simply doing a loop:
> 
> if (position == boilerplate_properties_) {
>   // Check if any of the remaining properties is __proto__:null.
>   if (!has_null_prototype()) {
>     for (; i < properties->length(); i++) {
>       if (properties()->at(i)->IsNullPrototype()) {
>         set_has_null_prototype(true);
>         break;
>       }
>     }
>   }
>   break;
> }
> 
> You could also have another flag "has_seen_prototype" in case the
IsBoilerplateProperty above hits; as an alternative to has_null_prototype().

Added separate helper method.
I will still iterate over the rest of the literal properties, given that you
might

https://codereview.chromium.org/2445333002/diff/340001/src/ast/ast.cc#newcode618
src/ast/ast.cc:618: bit_field_ = HasElementsField::update(bit_field_, elements >
0);
On 2017/03/20 at 14:47:11, Toon Verwaest wrote:
> Leftover code?

removed.

[Toon Verwaest, 2017-04-26 12:28:15]

lgtm

https://codereview.chromium.org/2445333002/diff/520001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2445333002/diff/520001/src/factory.cc#newcode...
src/factory.cc:2681: // Ignoring number_of_properties for force dictionary map
with __proto__:null.
It's kinda silly to pass has_null_prototype through here if we're just returning
something from the native context. What about moving this branch outside and
only calling ObjectLiteralMapFromCache if we're retrieving it from the cache?

[commit-bot: I haz the power, 2017-04-26 12:28:48]

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

[commit-bot: I haz the power, 2017-04-26 12:28:49]

Dry run: This issue passed the CQ dry run.

[adamk, 2017-04-26 18:48:15]

I'm mostly deferring to Toon on the code, so this lgtm from my point of view,
but please update the documentation here (both the comments mentioned below and
the CL description, which doesn't talk about lots of the things happening in
this patch).

https://codereview.chromium.org/2445333002/diff/520001/src/ast/compile-time-v...
File src/ast/compile-time-value.h (right):

https://codereview.chromium.org/2445333002/diff/520001/src/ast/compile-time-v...
src/ast/compile-time-value.h:20: static const int kArrayLiteralFlag = -1;
This could use a comment, and see below.

https://codereview.chromium.org/2445333002/diff/520001/src/ast/compile-time-v...
src/ast/compile-time-value.h:27: // Get the type of a compile time value
returned by GetValue().
Please update this comment to explain what this thing returns. What flags are
these?

[Camillo Bruni, 2017-04-27 09:32:45]

Description was changed from

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
With this CL the EmitFastCloneShallowObject builtin supports cloning slow
object boilerplates. Furthermore this CL unifies behavior to find the matching
Map and instantiating it for Object.create(null) and literals with a null
prototype.
==========

to

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
- The EmitFastCloneShallowObject builtin now supports cloning slow
  object boilerplates. 
- Unified behavior to find the matching Map and instantiating it for 
  Object.create(null) and literals with a null prototype.
- Cleanup of literal type parameter of CompileTimeValue, now in sync with 
  ObjectLiteral flags.
==========

[Camillo Bruni, 2017-04-27 09:33:28]

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

[commit-bot: I haz the power, 2017-04-27 09:33:40]

Dry run: CQ is trying da patch.

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

[Camillo Bruni, 2017-04-27 09:39:41]

https://codereview.chromium.org/2445333002/diff/520001/src/ast/compile-time-v...
File src/ast/compile-time-value.h (right):

https://codereview.chromium.org/2445333002/diff/520001/src/ast/compile-time-v...
src/ast/compile-time-value.h:20: static const int kArrayLiteralFlag = -1;
On 2017/04/26 at 18:48:15, adamk wrote:
> This could use a comment, and see below.

Added comment + DCHECK to better explain the purpose of this value.

https://codereview.chromium.org/2445333002/diff/520001/src/ast/compile-time-v...
src/ast/compile-time-value.h:27: // Get the type of a compile time value
returned by GetValue().
On 2017/04/26 at 18:48:14, adamk wrote:
> Please update this comment to explain what this thing returns. What flags are
these?

updated with explanation of where the values come from.

https://codereview.chromium.org/2445333002/diff/520001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2445333002/diff/520001/src/factory.cc#newcode...
src/factory.cc:2681: // Ignoring number_of_properties for force dictionary map
with __proto__:null.
On 2017/04/26 at 12:28:15, Toon Verwaest wrote:
> It's kinda silly to pass has_null_prototype through here if we're just
returning something from the native context. What about moving this branch
outside and only calling ObjectLiteralMapFromCache if we're retrieving it from
the cache?

you're right, pulled check up and removed args.

[commit-bot: I haz the power, 2017-04-27 10:09:47]

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

[commit-bot: I haz the power, 2017-04-27 10:09:49]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2017-04-27 10:27:17]

https://codereview.chromium.org/2445333002/diff/540001/src/runtime/runtime-li...
File src/runtime/runtime-literals.cc (right):

https://codereview.chromium.org/2445333002/diff/540001/src/runtime/runtime-li...
src/runtime/runtime-literals.cc:37:
handle(native_context->slow_object_with_null_prototype_map(), isolate);
map = isolate->slow_object_with_null_prototype_map() should work

or just:

Handle<Map> map = has_null_prototype
  ? isolate->slow_object_with_null_prototype_map()
  : isolate->factory()->ObjectLiteralMapFromCache(native_context,
number_of_properties);

[Camillo Bruni, 2017-04-27 12:08:36]

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

[Camillo Bruni, 2017-04-27 12:26:34]

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

[commit-bot: I haz the power, 2017-04-27 12:26:39]

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, 2017-04-27 12:30:08]

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

[commit-bot: I haz the power, 2017-04-27 12:30:09]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  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...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/24985)

[Camillo Bruni, 2017-04-27 12:35:39]

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

[commit-bot: I haz the power, 2017-04-27 12:35:46]

Dry run: CQ is trying da patch.

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

[mythria, 2017-04-27 12:36:04]

lgtm on interpreter

[commit-bot: I haz the power, 2017-04-27 13:03:11]

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

[commit-bot: I haz the power, 2017-04-27 13:03:12]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-04-27 14:44:07]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2017-04-27 14:44:07]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2445333002/#ps580001
(title: "fixing compilation issue")

[commit-bot: I haz the power, 2017-04-27 14:44:17]

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, 2017-04-27 14:48:34]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1493304247172060,
"parent_rev": "dc713be87d59d1ca6a2735b976559cc0d2127025", "commit_rev":
"3f73fecb132457ba66b1cfbd04a025b33ffef3b7"}

[commit-bot: I haz the power, 2017-04-27 14:48:41]

Description was changed from

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
- The EmitFastCloneShallowObject builtin now supports cloning slow
  object boilerplates. 
- Unified behavior to find the matching Map and instantiating it for 
  Object.create(null) and literals with a null prototype.
- Cleanup of literal type parameter of CompileTimeValue, now in sync with 
  ObjectLiteral flags.
==========

to

==========
[runtime] Ensure slow properties for simple {__proto__:null} literals.

With this CL we reduce the difference between directly using a null prototype
in a literal or using Object.create(null).
- The EmitFastCloneShallowObject builtin now supports cloning slow
  object boilerplates. 
- Unified behavior to find the matching Map and instantiating it for 
  Object.create(null) and literals with a null prototype.
- Cleanup of literal type parameter of CompileTimeValue, now in sync with 
  ObjectLiteral flags.

Review-Url: https://codereview.chromium.org/2445333002
Cr-Commit-Position: refs/heads/master@{#44941}
Committed:
https://chromium.googlesource.com/v8/v8/+/3f73fecb132457ba66b1cfbd04a025b33ff...
==========

[commit-bot: I haz the power, 2017-04-27 14:48:43]

Committed patchset #30 (id:580001) as
https://chromium.googlesource.com/v8/v8/+/3f73fecb132457ba66b1cfbd04a025b33ff...