Move Oddball/String to %Typearray%.prototype.fill fast path

Move Oddball/String to %Typearray%.prototype.fill fast path

ToNumber for Oddball/String has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929, chromium:702902
Review-Url: https://codereview.chromium.org/2769673002
Cr-Commit-Position: refs/heads/master@{#44129}
Committed: https://chromium.googlesource.com/v8/v8/+/a1f2239e0b7daff6bbf728d4f744ef6ee52f5299

[rongjie, 2017-03-22 13:05:50]

loorongjie@gmail.com changed reviewers:
+ bmeurer@chromium.org, caitp@igalia.com, cbruni@chromium.org

[rongjie, 2017-03-22 13:08:51]

Description was changed from

==========
Move Boolean/null/undefined to TA.p.fill fast path

ToNumber for Boolean/null/undefined has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

to

==========
Move Boolean/null/undefined to TA.p.fill fast path

ToNumber for Boolean/null/undefined has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

[caitp, 2017-03-22 13:10:51]

I think it would be fine to only handle undefined on the fast path. Maybe `null`
too, but I'm not sure using "true"/"false" as parameters to TA.p.fill makes a
whole lot of sense. I'd even be fine with restricting the fast path to numbers
that are Smis.

But, that's just my opinion.

[caitp, 2017-03-22 13:11:42]

https://codereview.chromium.org/2769673002/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/2769673002/diff/1/src/elements.cc#newcode2869
src/elements.cc:2869: } else if (obj_value->IsHeapNumber()) {
Oh nvm, I thought we had a fast path in CSA. This is fine

[Camillo Bruni, 2017-03-22 21:26:50]

almost :)

https://codereview.chromium.org/2769673002/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/2769673002/diff/1/src/elements.cc#newcode2869
src/elements.cc:2869: } else if (obj_value->IsHeapNumber()) {
Given that we now perform additional checks, let's avoid the additional implied
IsHeapObject() tests further down by casting obj_value:

if (obj_value->IsSmi()) {
...
} else {
  Handle<HeapObject> heap_obj_value = Handle<HeapObject>::cast(obj_value);
  if (heap_obj_value->IsHeapNumber()) { ... }
  else if (heap_obj_value->IsNull(isolate) { .. }
  ...

}

https://codereview.chromium.org/2769673002/diff/1/test/mjsunit/es6/typedarray...
File test/mjsunit/es6/typedarray-fill.js (right):

https://codereview.chromium.org/2769673002/diff/1/test/mjsunit/es6/typedarray...
test/mjsunit/es6/typedarray-fill.js:45: assertArrayEquals([0, 0, 0, 0, 0], new
constructor([0, 0, 0, 0, 0]).fill(null));
please add the same for undefined :)

[rongjie, 2017-03-23 00:01:49]

https://codereview.chromium.org/2769673002/diff/1/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/2769673002/diff/1/src/elements.cc#newcode2869
src/elements.cc:2869: } else if (obj_value->IsHeapNumber()) {
On 2017/03/22 21:26:50, Camillo Bruni wrote:
> Given that we now perform additional checks, let's avoid the additional
implied
> IsHeapObject() tests further down by casting obj_value:
> 
> if (obj_value->IsSmi()) {
> ...
> } else {
>   Handle<HeapObject> heap_obj_value = Handle<HeapObject>::cast(obj_value);
>   if (heap_obj_value->IsHeapNumber()) { ... }
>   else if (heap_obj_value->IsNull(isolate) { .. }
>   ...
> 
> }

Done.

https://codereview.chromium.org/2769673002/diff/1/test/mjsunit/es6/typedarray...
File test/mjsunit/es6/typedarray-fill.js (right):

https://codereview.chromium.org/2769673002/diff/1/test/mjsunit/es6/typedarray...
test/mjsunit/es6/typedarray-fill.js:45: assertArrayEquals([0, 0, 0, 0, 0], new
constructor([0, 0, 0, 0, 0]).fill(null));
On 2017/03/22 21:26:50, Camillo Bruni wrote:
> please add the same for undefined :)

Done.

[Camillo Bruni, 2017-03-23 10:56:43]

sorry.. another comment

https://codereview.chromium.org/2769673002/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/2769673002/diff/40001/src/elements.cc#newcode...
src/elements.cc:2876:
BackingStore::from_double(std::numeric_limits<double>::quiet_NaN());
I just realized that we do already have a number cache on the oddball objects
(True, false, undefined).
So you can do, the following (and reuse some more code):

    if (input->IsOddball()) {
      value =  BackingStore::from_double(
                 Oddball::ToNumber(Handle<Oddball>::cast(input)->Number());
    } else if (input->IsString()) {
      value = BackingStore::from_double(
                 String::ToNumber(Handle<String>::cast(input))->Number());
    }

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

https://codereview.chromium.org/2769673002/diff/40001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/2769673002/diff/40001/src/elements.cc#newcode...
src/elements.cc:2876:
BackingStore::from_double(std::numeric_limits<double>::quiet_NaN());
On 2017/03/23 10:56:43, Camillo Bruni wrote:
> I just realized that we do already have a number cache on the oddball objects
> (True, false, undefined).
> So you can do, the following (and reuse some more code):
> 
>     if (input->IsOddball()) {
>       value =  BackingStore::from_double(
>                  Oddball::ToNumber(Handle<Oddball>::cast(input)->Number());
>     } else if (input->IsString()) {
>       value = BackingStore::from_double(
>                  String::ToNumber(Handle<String>::cast(input))->Number());
>     }

Done.

[rongjie, 2017-03-24 08:38:23]

Description was changed from

==========
Move Boolean/null/undefined to TA.p.fill fast path

ToNumber for Boolean/null/undefined has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

to

==========
ToNumber for Oddball/String has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

[rongjie, 2017-03-24 08:39:21]

Description was changed from

==========
ToNumber for Oddball/String has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

to

==========
Move Oddball/String to %Typearray%.prototype.fill fast path

ToNumber for Oddball/String has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

[Camillo Bruni, 2017-03-24 13:43:53]

LGTM, please wait for lgtm from bmeurer

[Benedikt Meurer, 2017-03-24 17:59:01]

lgtm

[rongjie, 2017-03-24 22:17:45]

The CQ bit was checked by loorongjie@gmail.com

[commit-bot: I haz the power, 2017-03-24 22:17: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, 2017-03-24 22:43:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490393865975840,
"parent_rev": "2beb56137fe4194a69686abdad4c9246cbc93fea", "commit_rev":
"a1f2239e0b7daff6bbf728d4f744ef6ee52f5299"}

[commit-bot: I haz the power, 2017-03-24 22:43:42]

Description was changed from

==========
Move Oddball/String to %Typearray%.prototype.fill fast path

ToNumber for Oddball/String has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902
==========

to

==========
Move Oddball/String to %Typearray%.prototype.fill fast path

ToNumber for Oddball/String has no side-effect, no need to go
through %Typearray%.prototype.fill slow path.

BUG=v8:5929,chromium:702902

Review-Url: https://codereview.chromium.org/2769673002
Cr-Commit-Position: refs/heads/master@{#44129}
Committed:
https://chromium.googlesource.com/v8/v8/+/a1f2239e0b7daff6bbf728d4f744ef6ee52...
==========

[commit-bot: I haz the power, 2017-03-24 22:43:44]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/a1f2239e0b7daff6bbf728d4f744ef6ee52...