Migrate Object constructor to C++

Migrate Object constructor to C++

BUG=v8:6005

[rongjie, 2017-03-23 05:32:43]

loorongjie@gmail.com changed reviewers:
+ bmeurer@chromium.org, yangguo@chromium.org

[rongjie, 2017-03-23 06:54:09]

I am getting some CHECK fails concerning Crankshaft:

out.gn\x64.debug\d8 --allow-natives-syntax --nofold-constants --crankshaft
--noturbo test\mjsunit\mjsunit.js test\mjsunit\regress\regress-crbug-119926.js


#
# Fatal error in d:\cr\v8\v8\src\crankshaft\hydrogen.cc, line 9781
# Check failed: constructor->shared()->construct_stub() ==
isolate()->builtins()->builtin(Builtins::kJSConstructStubGeneric) ||
constructor->shared()->construct_stub()
== isolate()->builtins()->builtin(Builtins::kJSConstructStubApi).
#

JSConstructStubGeneric and JSConstructStubApi are implemented as ASM in
src/builtins/<arch>/builtins-<arch>.cc. Not sure how to fix this. I should
implement ObjectConstructor_ConstructStub in TFJ?

[Benedikt Meurer, 2017-03-23 07:11:13]

Awesome, lgtm​!

[rongjie, 2017-03-23 07:14:39]

On 2017/03/23 07:11:13, Benedikt Meurer wrote:
> Awesome, lgtm​!

? Many tests with --noturbo --crankshaft still fail. I still need to know how to
fix it. Any idea?

[rongjie, 2017-03-23 07:49:44]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 07: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-23 08:06:02]

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

[commit-bot: I haz the power, 2017-03-23 08:06:03]

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...)

[rongjie, 2017-03-23 09:56:54]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 09:57:00]

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-23 10:12:00]

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

[commit-bot: I haz the power, 2017-03-23 10:12:01]

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...)

[rongjie, 2017-03-23 10:49:58]

My previous ObjectConstructor_ConstructStub was not following the spec
correctly, fixed now.

ObjectConstructor and ObjectConstructor_ConstructStub now share a lot of code,
probably should merge them into one BUILTIN function.

Weird thing is, many mjsunit CHECK fails related to Crankshaft I see in local
build do not appear in tryjob.
https://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trig...

There are a few more tests failed in tryjob, one is mjsunit/elements-kind.js. If
I comment out the following code (which fails and I have no idea how my change
causes this):

var me = {};
assertKind(elements_kind.fast, me);
me.dance = 0xD15C0;
me.drink = 0xC0C0A;
assertKind(elements_kind.fast, me);

I get "Fatal error in d:\cr\v8\v8\src\crankshaft\hydrogen.cc, line 9781" like in
comment#2.

I wonder if those failed unittest and cctest tests are related to Crankshaft as
well? If not, some pointers to fix them?

[Benedikt Meurer, 2017-03-23 11:41:19]

bmeurer@chromium.org changed reviewers:
+ mvstanton@chromium.org

[Benedikt Meurer, 2017-03-23 11:41:19]

bmeurer@chromium.org changed required reviewers:
+ mvstanton@chromium.org

[Benedikt Meurer, 2017-03-23 11:41:20]

+mvstanton

[rongjie, 2017-03-28 07:59:36]

Description was changed from

==========
Migrate Object constructor to C++

BUG=v8:6005
==========

to

==========
Migrate Object constructor to C++

BUG=v8:6005
==========

[rongjie, 2017-03-28 07:59:36]

loorongjie@gmail.com changed reviewers:
+ danno@chromium.org, jarin@chromium.org

[rongjie, 2017-03-28 08:05:56]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 08:06:03]

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-28 08:06:04]

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

[commit-bot: I haz the power, 2017-03-28 08:06:05]

Dry run: All required reviewers (with asterisk prefixes) have not yet approved
this CL.

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[rongjie, 2017-03-28 08:13:42]

loorongjie@gmail.com changed required reviewers:
- mvstanton@chromium.org

[rongjie, 2017-03-28 08:15:45]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 08:15:53]

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-28 08:16:23]

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

[commit-bot: I haz the power, 2017-03-28 08:16:24]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, build hasn't started yet,
builder probably lacks capacity)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, build hasn't started yet,
builder probably lacks capacity)

[rongjie, 2017-03-28 08:48:42]

(Added two more crankshaft owners)
(Sorry mvstanton@, I removed the asterisk to get tryjob. I will still wait for
you)

Hi danno and jarin,

My CL is largely based on [1].

Here are the tests that has this CHECK (src\crankshaft\hydrogen.cc:9780) fails
with --no-turbo (run fine without this flag).

constructor->shared()->construct_stub() ==
isolate()->builtins()->builtin(Builtins::kJSConstructStubGeneric) ||
constructor->shared()->construct_stub() ==
isolate()->builtins()->builtin(Builtins::kJSConstructStubApi)

cctest/test-api-interceptors/InterceptorLoadICFieldNotNeeded
mjsunit/boolean
mjsunit/object-literal
webkit/fast/js/kde/object_prototype_tostring
mjsunit/regress/regress-1254366
mjsunit/regress/regress-1650
mjsunit/regress/regress-2291
mjsunit/regress/regress-349885
mjsunit/string-compare-alignment
mjsunit/array-literal-transitions
mjsunit/array-store-and-grow
mjsunit/elements-transition-hoisting

I couldn't figure out what to do to make crankshaft happy. Can you advice?

There are two other tests that fail (but not CHECK fail as above):

cctest/test-inobject-slack-tracking/ObjectLiteralPropertyBackingStoreSize
mjsunit/elements-kind

Note: If I removed [2] from mjsunit/elements-kind, I get the CHECK fail as
above.

- Rong Jie

[1]:
https://chromium.googlesource.com/v8/v8.git/+/8f87c0acb7f241654cac9574b7ec61e...
[2]:
https://cs.chromium.org/chromium/src/v8/test/mjsunit/elements-kind.js?l=85-89

[rongjie, 2017-03-29 11:45:24]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 11:45:35]

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-29 11:47:05]

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

[commit-bot: I haz the power, 2017-03-29 11:47:06]

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

[rongjie, 2017-03-29 11:53:34]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 11:53:45]

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-29 11:56:15]

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

[commit-bot: I haz the power, 2017-03-29 11:56:16]

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_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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23504)
  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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/23531)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/23419)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19997)

[rongjie, 2017-03-29 12:11:05]

Patchset #5 (id:80001) has been deleted

[rongjie, 2017-03-29 12:11:36]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 12:11:46]

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-29 12:28:16]

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

[commit-bot: I haz the power, 2017-03-29 12:28:17]

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...)

[rongjie, 2017-03-30 02:21:33]

The CQ bit was checked by loorongjie@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 02:21:37]

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-30 02:38:10]

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

[commit-bot: I haz the power, 2017-03-30 02:38:11]

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...)

[rongjie, 2017-03-30 04:23:48]

This is my last attempt to ask help in this CL.

- Adding Builtins::kObjectConstructor_ConstructStub into the DCHECK fixes all
src/crankshaft/hydrogen.cc crashes without making extra tests to fail.
- Manually do object->map()->set_elements_kind(FAST_ELEMENTS) fixes
mjsunit/elements-kind

I now left we 2 more tests:

cctest/test-inobject-slack-tracking/ObjectLiteralPropertyBackingStoreSize
- The test is about empty object literal should 4 "slack" properties.
- I thought adding TENURED to NewJSObject will work, it didn't.
- object->map()->SetInObjectProperties(4) and
object->map()->set_unused_property_fields(4) will cause crashing.
- Map::EnsureDescriptorSlack(map, 4) does not affect GetInObjectProperties().
- Scanned all files containing "slack" or "4", no luck.

unittests/JSCreateLoweringTest.JSCreateArgumentsInlinedRestArray
- All I know is a DCHECK(current_depth_ >= 0) in
StateValuesAccess::iterator::Top fails. Stack trace gives a list of nameless
symbols, I don't even know where to start.

Guidance to fix them will be much appreciated.

[Benedikt Meurer, 2017-03-30 06:35:16]

Sorry for the delay. Will take a look now.

[Benedikt Meurer, 2017-03-30 08:33:22]

Unfortunately it seems like there seems to be a fundamental issue here, where we
now share the initial map of the Object function with the empty literal, which
confuses the slack tracking. I'm not sure how to resolve this, but it seems that
this is definitely not trivial.

So, sorry for the delay, but it seems that this is unfortunately not a good
starter project. Sorry.

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
src/builtins/builtins-object.cc:18: static Object*
ObjectConstructCommon(Isolate* isolate, Handle<Object> value) {
Nit: Instead of static put this into an anonymous namespace.

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
src/builtins/builtins-object.cc:21:
isolate->factory()->NewJSObject(isolate->object_function(), TENURED);
Don't use TENURED here, just omit the second parameter.

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
src/builtins/builtins-object.cc:22:
object->map()->set_elements_kind(FAST_ELEMENTS);
Remove this line.

[rongjie, 2017-03-30 09:08:22]

Thanks for taking a look!

> Unfortunately it seems like there seems to be a fundamental issue here, where
we now share the initial map of the Object function with the empty literal,
which confuses the slack tracking.

I see some places where when the spec uses ObjectCreate(%ObjectPrototype%), V8
uses isolate->factory()->NewJSObject(isolate->object_function()). If this is
wrong, some tests should fail before this?

https://tc39.github.io/ecma262/#sec-object.getownpropertydescriptors
https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-object.cc?type=...

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
File src/builtins/builtins-object.cc (right):

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
src/builtins/builtins-object.cc:18: static Object*
ObjectConstructCommon(Isolate* isolate, Handle<Object> value) {
On 2017/03/30 08:33:22, Benedikt Meurer wrote:
> Nit: Instead of static put this into an anonymous namespace.

Done.

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
src/builtins/builtins-object.cc:21:
isolate->factory()->NewJSObject(isolate->object_function(), TENURED);
On 2017/03/30 08:33:22, Benedikt Meurer wrote:
> Don't use TENURED here, just omit the second parameter.

Done.

https://codereview.chromium.org/2770753003/diff/120001/src/builtins/builtins-...
src/builtins/builtins-object.cc:22:
object->map()->set_elements_kind(FAST_ELEMENTS);
On 2017/03/30 08:33:22, Benedikt Meurer wrote:
> Remove this line.

Done.
That was temp fix for mjsunit/elements-kind.

[Benedikt Meurer, 2017-03-30 09:41:34]

On 2017/03/30 09:08:22, rongjie wrote:
> Thanks for taking a look!
> 
> > Unfortunately it seems like there seems to be a fundamental issue here,
where
> we now share the initial map of the Object function with the empty literal,
> which confuses the slack tracking.
> 
> I see some places where when the spec uses ObjectCreate(%ObjectPrototype%), V8
> uses isolate->factory()->NewJSObject(isolate->object_function()). If this is
> wrong, some tests should fail before this?
> 
> https://tc39.github.io/ecma262/#sec-object.getownpropertydescriptors
>
https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-object.cc?type=...

Yes. I'm not sure what the right approach will be to address this. We will have
to discuss this on the team first. Let's hold off on porting the Object
constructor for now, but please keep the CL up for future reference. And thanks
for the work/patience with this.