Rewrite Object.prototype.toString in C++

Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

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

[adamk, 2015-12-07 23:53:18]

Description was changed from

==========
Work in progress: Rewrite Object.prototype.toString in C++

BUG=chromium:555127, v8:3502
LOG=n
==========

to

==========
Work in progress: Rewrite Object.prototype.toString in C++

Also fixes the API version of toString to handle the "arguments" object
correctly per ES5 (following the JS fix in
https://code.google.com/p/v8/source/detail?r=3279).

BUG=chromium:555127, v8:3502
LOG=n
==========

[adamk, 2015-12-08 01:15:23]

Description was changed from

==========
Work in progress: Rewrite Object.prototype.toString in C++

Also fixes the API version of toString to handle the "arguments" object
correctly per ES5 (following the JS fix in
https://code.google.com/p/v8/source/detail?r=3279).

BUG=chromium:555127, v8:3502
LOG=n
==========

to

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
==========

[adamk, 2015-12-08 01:16:38]

adamk@chromium.org changed reviewers:
+ hpayer@chromium.org, verwaest@chromium.org

[adamk, 2015-12-08 01:16:38]

verwaest: the whole thing. In particular, not sure I put
Runtime::ObjectProtoToString in the right place. Should it just in in
objects.cc?

hpayer: heap OWNERS

[caitp (gmail), 2015-12-08 01:23:01]

On 2015/12/08 01:16:38, adamk wrote:
> verwaest: the whole thing. In particular, not sure I put
> Runtime::ObjectProtoToString in the right place. Should it just in in
> objects.cc?
> 
> hpayer: heap OWNERS

The API function used to always return a flat string, while this one returns a
rope. Does this matter?

[adamk, 2015-12-08 01:28:54]

On 2015/12/08 01:23:01, caitp wrote:
> On 2015/12/08 01:16:38, adamk wrote:
> > verwaest: the whole thing. In particular, not sure I put
> > Runtime::ObjectProtoToString in the right place. Should it just in in
> > objects.cc?
> > 
> > hpayer: heap OWNERS
> 
> The API function used to always return a flat string, while this one returns a
> rope. Does this matter?

I hope not; the API doesn't distinguish between the two. I used NewConsString
mostly for simplicity.

[Benedikt Meurer, 2015-12-08 05:26:21]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[Benedikt Meurer, 2015-12-08 05:26:21]

I like the approach. How about using the IncrementalStringBuilder? And I'm not a
big fan of putting stuff into Runtime class; how about moving it to
JSObject::ToString?

[Michael Starzinger, 2015-12-08 08:16:48]

On 2015/12/08 05:26:21, Benedikt Meurer wrote:
> I like the approach. How about using the IncrementalStringBuilder? And I'm not
a
> big fan of putting stuff into Runtime class; how about moving it to
> JSObject::ToString?

+1. Pretty please, not on the Runtime class, I am working hard to remove the
last remaining methods from there.

[Hannes Payer (out of office), 2015-12-08 14:12:39]

https://codereview.chromium.org/1509533003/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1509533003/diff/20001/src/heap/heap.h#newcode325
src/heap/heap.h:325: V(null_to_string, "[object Null]")                         
   \
alphabetic order

[adamk, 2015-12-08 23:19:52]

Okay, handled comments:
  - Used IncrementalStringBuilder (thanks for the pointer, didn't know we had
that).
  - Moved to JSObject. Longer term I think we might want to move more of the
spec implementation stuff out of objects.cc and into something else (I know
jkummerow proposed something of this sort awhile ago).

https://codereview.chromium.org/1509533003/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1509533003/diff/20001/src/heap/heap.h#newcode325
src/heap/heap.h:325: V(null_to_string, "[object Null]")                         
   \
On 2015/12/08 14:12:39, Hannes Payer wrote:
> alphabetic order

Done.

[Benedikt Meurer, 2015-12-09 04:40:25]

On 2015/12/08 23:19:52, adamk wrote:
> Okay, handled comments:
>   - Used IncrementalStringBuilder (thanks for the pointer, didn't know we had
> that).
>   - Moved to JSObject. Longer term I think we might want to move more of the
> spec implementation stuff out of objects.cc and into something else (I know
> jkummerow proposed something of this sort awhile ago).

Hm, yeah, at least for whole builtins being written in C++ that would make
sense. Maybe something like

(1) src/builtins folder with all stuff in v8::internal::builtins namespace,
(2) src/builtins/object-prototype.{cc,h} with class
v8::internal::builtins::ObjectPrototype for everything that lives in
Object.prototype
(3) src/builtins/reflect.{cc,h} with class v8::internal::builtins::Reflect for
everything in Reflect
and so on

The classes would contain only handlified static methods. Does that sound like a
reasonable, non-intrusive plan?

[adamk, 2015-12-09 20:21:16]

On 2015/12/09 04:40:25, Benedikt Meurer wrote:
> On 2015/12/08 23:19:52, adamk wrote:
> > Okay, handled comments:
> >   - Used IncrementalStringBuilder (thanks for the pointer, didn't know we
had
> > that).
> >   - Moved to JSObject. Longer term I think we might want to move more of the
> > spec implementation stuff out of objects.cc and into something else (I know
> > jkummerow proposed something of this sort awhile ago).
> 
> Hm, yeah, at least for whole builtins being written in C++ that would make
> sense. Maybe something like
> 
> (1) src/builtins folder with all stuff in v8::internal::builtins namespace,
> (2) src/builtins/object-prototype.{cc,h} with class
> v8::internal::builtins::ObjectPrototype for everything that lives in
> Object.prototype
> (3) src/builtins/reflect.{cc,h} with class v8::internal::builtins::Reflect for
> everything in Reflect
> and so on
> 
> The classes would contain only handlified static methods. Does that sound like
a
> reasonable, non-intrusive plan?

Something like that seems like a reasonable plan. Happy to do that in a separate
round of refactoring (I'd also move all the internal methods that are right now
hanging off of v8::internal::Object, for example).

[Michael Starzinger, 2015-12-09 20:30:11]

On 2015/12/09 20:21:16, adamk wrote:
> On 2015/12/09 04:40:25, Benedikt Meurer wrote:
> > On 2015/12/08 23:19:52, adamk wrote:
> > > Okay, handled comments:
> > >   - Used IncrementalStringBuilder (thanks for the pointer, didn't know we
> had
> > > that).
> > >   - Moved to JSObject. Longer term I think we might want to move more of
the
> > > spec implementation stuff out of objects.cc and into something else (I
know
> > > jkummerow proposed something of this sort awhile ago).
> > 
> > Hm, yeah, at least for whole builtins being written in C++ that would make
> > sense. Maybe something like
> > 
> > (1) src/builtins folder with all stuff in v8::internal::builtins namespace,
> > (2) src/builtins/object-prototype.{cc,h} with class
> > v8::internal::builtins::ObjectPrototype for everything that lives in
> > Object.prototype
> > (3) src/builtins/reflect.{cc,h} with class v8::internal::builtins::Reflect
for
> > everything in Reflect
> > and so on
> > 
> > The classes would contain only handlified static methods. Does that sound
like
> a
> > reasonable, non-intrusive plan?
> 
> Something like that seems like a reasonable plan. Happy to do that in a
separate
> round of refactoring (I'd also move all the internal methods that are right
now
> hanging off of v8::internal::Object, for example).

Please propose a concrete plan about this in a separate thread on v8-dev@ before
submitting CLs in that regard. I am fine with what is mentioned here, as long as
none of the write-barrier related macros are duplicated and the low-level object
accessors using them are not spread around. But I am sure others have an opinion
about this too and might have missed this hidden discussion.

[adamk, 2015-12-09 20:38:41]

On 2015/12/09 20:30:11, Michael Starzinger wrote:
> On 2015/12/09 20:21:16, adamk wrote:
> > On 2015/12/09 04:40:25, Benedikt Meurer wrote:
> > > On 2015/12/08 23:19:52, adamk wrote:
> > > > Okay, handled comments:
> > > >   - Used IncrementalStringBuilder (thanks for the pointer, didn't know
we
> > had
> > > > that).
> > > >   - Moved to JSObject. Longer term I think we might want to move more of
> the
> > > > spec implementation stuff out of objects.cc and into something else (I
> know
> > > > jkummerow proposed something of this sort awhile ago).
> > > 
> > > Hm, yeah, at least for whole builtins being written in C++ that would make
> > > sense. Maybe something like
> > > 
> > > (1) src/builtins folder with all stuff in v8::internal::builtins
namespace,
> > > (2) src/builtins/object-prototype.{cc,h} with class
> > > v8::internal::builtins::ObjectPrototype for everything that lives in
> > > Object.prototype
> > > (3) src/builtins/reflect.{cc,h} with class v8::internal::builtins::Reflect
> for
> > > everything in Reflect
> > > and so on
> > > 
> > > The classes would contain only handlified static methods. Does that sound
> like
> > a
> > > reasonable, non-intrusive plan?
> > 
> > Something like that seems like a reasonable plan. Happy to do that in a
> separate
> > round of refactoring (I'd also move all the internal methods that are right
> now
> > hanging off of v8::internal::Object, for example).
> 
> Please propose a concrete plan about this in a separate thread on v8-dev@
before
> submitting CLs in that regard. I am fine with what is mentioned here, as long
as
> none of the write-barrier related macros are duplicated and the low-level
object
> accessors using them are not spread around. But I am sure others have an
opinion
> about this too and might have missed this hidden discussion.

Indeed that would be my plan (by "separate round of refactoring" I mostly meant
I didn't want to do anything in this patch, not that I intended to follow up
with CLs).

In general I think this is just a continuation of this thread:
https://groups.google.com/d/msg/v8-dev/vAdxba3RG6I/xGXCmm9GBgAJ

[adamk, 2015-12-10 17:29:04]

Ping? Would love to get this in to confirm that it gets the same perf win on the
bots that I see locally.

[Hannes Payer (out of office), 2015-12-10 18:33:01]

heap LGTM

[Michael Starzinger, 2015-12-10 18:41:22]

Unsure whether I was meant as a reviewer for this, but LGTM on "runtime" FWIW.
Didn't look at the rest at all.

[adamk, 2015-12-10 19:24:40]

On 2015/12/10 18:41:22, Michael Starzinger wrote:
> Unsure whether I was meant as a reviewer for this, but LGTM on "runtime" FWIW.
> Didn't look at the rest at all.

Didn't realize how many reviewers had piled up here. I'm looking for an lgtm
from Toon.

[Toon Verwaest, 2015-12-10 20:13:17]

Lgtm as well. I added a side effect free copy of this method to messages.js in
the meantime, using the flag you are removing. I'm fine with removing that
branch as well, since it's only used for uncaught exception rendering.

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

https://codereview.chromium.org/1509533003/diff/40001/src/objects.cc#newcode1...
src/objects.cc:16104: 
Class-name is fairly expensive since it needs to find the constructor by
following all the back pointers in the transition tree. We might want to only do
that if no string tag is set. Later perhaps we should rewrite it to use the
instance type...

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

https://codereview.chromium.org/1509533003/diff/40001/src/objects.h#newcode2485
src/objects.h:2485: Isolate* isolate, Handle<Object> object);
A little odd to drop Object in the method-name...

[adamk, 2015-12-10 23:22:46]

Description was changed from

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
==========

to

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

[adamk, 2015-12-10 23:25:29]

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

https://codereview.chromium.org/1509533003/diff/40001/src/objects.cc#newcode1...
src/objects.cc:16104: 
On 2015/12/10 20:13:17, Toon Verwaest wrote:
> Class-name is fairly expensive since it needs to find the constructor by
> following all the back pointers in the transition tree. We might want to only
do
> that if no string tag is set. Later perhaps we should rewrite it to use the
> instance type...

Moved to call it only if no @@toStringTag and added a TODO to replace it with
instance type checks.

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

https://codereview.chromium.org/1509533003/diff/40001/src/objects.h#newcode2485
src/objects.h:2485: Isolate* isolate, Handle<Object> object);
On 2015/12/10 20:13:17, Toon Verwaest wrote:
> A little odd to drop Object in the method-name...

Fair, I'll put it back in.

[adamk, 2015-12-10 23:25:36]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-12-10 23:25:36]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
verwaest@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1509533003/#ps60001
(title: "verwaest comments")

[commit-bot: I haz the power, 2015-12-10 23:25:41]

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

[commit-bot: I haz the power, 2015-12-10 23:26:46]

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

[commit-bot: I haz the power, 2015-12-10 23:26:48]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  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_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11333)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[adamk, 2015-12-11 01:08:07]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-12-11 01:08:07]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
verwaest@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1509533003/#ps80001
(title: "Rebase, add back experimental flag")

[commit-bot: I haz the power, 2015-12-11 01:08:16]

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

[adamk, 2015-12-11 02:19:45]

The CQ bit was unchecked by adamk@chromium.org

[adamk, 2015-12-11 02:20:16]

Marked blink test for rebaselining in https://codereview.chromium.org/1513143005

[adamk, 2015-12-11 02:20:25]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-12-11 02:20:30]

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

[commit-bot: I haz the power, 2015-12-11 04:26:38]

Description was changed from

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

to

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-12-11 04:26:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-11 04:26:48]

Description was changed from

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel
==========

to

==========
Rewrite Object.prototype.toString in C++

The main impetus is to improve performance when --harmony-tostring
is enabled, thanks to using a generic property load instead of a
megamorphic IC.

This also reduces duplication, as the API function
v8::Object::ObjectProtoToString can share the runtime implementation.

The only functional change in this patch is to drop an accidental difference
between the JS and API implementations: the arguments object should toString
as "[object Arguments]". The JS side was corrected in
https://code.google.com/p/v8/source/detail?r=3279, but the API version was
missed in that patch.

BUG=chromium:555127, v8:3502
LOG=n
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2015-12-11 04:26:49]

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