Project: v8 Issues People Development process History Sign in
New issue
Advanced search Search tips
Starred by 7 users
Status: Assigned
Owner:
Cc:
Components:
HW: All
OS: All
Priority: 2
Type: FeatureRequest

Blocked on:
issue 5953
issue 5925



Sign in to add a comment
Slow TypedArray builtins
Project Member Reported by bmeu...@chromium.org, Feb 5 2017 Back to list
Most of the TypedArray builtins (including the TypedArray constructor) are still written in JavaScript with some weird fast paths. This makes them unnecessarily slow (and generic). We should rewrite the majority of them as C++ builtins, and some, especially those that call back into JavaScript, as CSA builtins.

This will boost their performance and make it more predictable in general. It should also help improve the maintainability long-term (reducing security risks, as the sometimes hard to understand JS<->Runtime interaction with the current typedarray.js has been the source of various security bugs).
 
Project Member Comment 1 by bugdroid1@chromium.org, Feb 6 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d36f14a73dbb7a907d0cdc6dd36ebd1781c1ba28

commit d36f14a73dbb7a907d0cdc6dd36ebd1781c1ba28
Author: caitp <caitp@igalia.com>
Date: Mon Feb 06 04:20:09 2017

[tests] add microbenchmark for %TypedArray%.prototype.copyWithin

Add a simple microbenchmark for calling copyWithin on a moderately large
Float64Array with 10,000 elements.

BUG=v8:5925, v8:5929, v8:4648
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org

Review-Url: https://codereview.chromium.org/2676193002
Cr-Commit-Position: refs/heads/master@{#42944}

[modify] https://crrev.com/d36f14a73dbb7a907d0cdc6dd36ebd1781c1ba28/test/js-perf-test/JSTests.json
[add] https://crrev.com/d36f14a73dbb7a907d0cdc6dd36ebd1781c1ba28/test/js-perf-test/TypedArrays/run.js
[add] https://crrev.com/d36f14a73dbb7a907d0cdc6dd36ebd1781c1ba28/test/js-perf-test/TypedArrays/typedarrays.js

Cc: danno@chromium.org
Project Member Comment 3 by bugdroid1@chromium.org, Feb 6 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/0f1c626d556cbf84b0e572635eb803729f88cbb3

commit 0f1c626d556cbf84b0e572635eb803729f88cbb3
Author: caitp <caitp@igalia.com>
Date: Mon Feb 06 17:45:14 2017

[typedarrays] move %TypedArray%.prototype.copyWithin to C++

- Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js
- Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which
relies on std::memmove rather than accessing individual eleements.
- Fixes the case where copyWithin is invoked on a TypedArray with a
detached buffer.
- Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the
algorithm

The C++ version gets through the benchmark more than 25000 times as
quickly as the JS implementation.

BUG=v8:5925, v8:5929, v8:4648
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org

Review-Url: https://codereview.chromium.org/2671233002
Cr-Commit-Position: refs/heads/master@{#42975}

[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/src/bootstrapper.cc
[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/src/builtins/builtins.h
[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/src/js/array.js
[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/src/js/typedarray.js
[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/test/mjsunit/es6/typedarray-copywithin.js
[modify] https://crrev.com/0f1c626d556cbf84b0e572635eb803729f88cbb3/test/test262/test262.status

Blockedon: 5953
caitp@, the patch seems to have a security hole. After validating typed array, ToInteger call can cause side effects.

For example,

var buf = new ArrayBuffer(100);
var arr = new Uint8Array(buf);
var tmp = {};
tmp[Symbol.toPrimitive] = function () {
  /* the buf can be neutered in here */
  return 123;
}
arr.copyWithin(tmp);
Comment 6 by ca...@igalia.com, Feb 12 2017
That demo script itself ends up returning early on builtins-typedarray.cc:245 (with the comment replaced with `%ArrayBufferNeuter(buf);`), so I'm not sure this is really a security issue,

but we could try to throw an exception if the buffer ends up neutered before the actual copy, with a second call to ValidateTypedArray().

Seems like a potential spec bug though, do you think you could file this on tc39/ecma262?
Oh, you should change "return 123" to "return 50", then it will call memmove.

var buf = new ArrayBuffer(0x10000);
var arr = new Uint8Array(buf).fill(55);
var tmp = {};
tmp[Symbol.toPrimitive] = function () {
  %ArrayBufferNeuter(arr.buffer);
  return 50;
}
arr.copyWithin(tmp);


I'm not familiar with spec things. Where do I need to file this issue on tc39/ecma262?
I'm not sure, but it seems that https://github.com/tc39/ecma262/issues/807 this issue is related.

ToInteger -> ToIndex.
Comment 10 by ca...@igalia.com, Feb 12 2017
Yeah, that is a problem if it can teach the memmove call. I'll send a fix on Monday, unless you'd like to send it
Project Member Comment 11 by bugdroid1@chromium.org, Feb 12 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/4530f0dc0c27233b05f852c29a6462ab67cbbad2

commit 4530f0dc0c27233b05f852c29a6462ab67cbbad2
Author: littledan <littledan@chromium.org>
Date: Sun Feb 12 21:16:18 2017

Revert of [typedarrays] move %TypedArray%.prototype.copyWithin to C++ (patchset #6 id:100001 of https://codereview.chromium.org/2671233002/ )

Reason for revert:
Due to security issue described in review thread.

Original issue's description:
> [typedarrays] move %TypedArray%.prototype.copyWithin to C++
>
> - Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js
> - Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which
> relies on std::memmove rather than accessing individual eleements.
> - Fixes the case where copyWithin is invoked on a TypedArray with a
> detached buffer.
> - Add tests to ensure that +/-Infinity (for all 3 parameters) is handled correctly by the
> algorithm
>
> The C++ version gets through the benchmark more than 25000 times as
> quickly as the JS implementation.
>
> BUG=v8:5925, v8:5929, v8:4648
> R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org
>
> Review-Url: https://codereview.chromium.org/2671233002
> Cr-Commit-Position: refs/heads/master@{#42975}
> Committed: https://chromium.googlesource.com/v8/v8/+/0f1c626d556cbf84b0e572635eb803729f88cbb3

TBR=cbruni@chromium.org,adamk@chromium.org,bmeurer@chromium.org,cwhan.tunz@gmail.com,caitp@igalia.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=v8:5925, v8:5929, v8:4648

Review-Url: https://codereview.chromium.org/2693753002
Cr-Commit-Position: refs/heads/master@{#43132}

[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/src/bootstrapper.cc
[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/src/builtins/builtins.h
[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/src/js/array.js
[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/src/js/typedarray.js
[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/test/mjsunit/es6/typedarray-copywithin.js
[modify] https://crrev.com/4530f0dc0c27233b05f852c29a6462ab67cbbad2/test/test262/test262.status

Project Member Comment 12 by bugdroid1@chromium.org, Feb 15 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/dc302c74be4183db1b84347afab893825c887add

commit dc302c74be4183db1b84347afab893825c887add
Author: caitp <caitp@igalia.com>
Date: Wed Feb 15 14:21:18 2017

Reland [typedarrays] move %TypedArray%.prototype.copyWithin to C++

- Removes shared InnerArrayCopyWithin JS builtin from src/js/array.js
- Implements %TypedArray%.prototype.copyWithin as a C++ builtin, which
relies on std::memmove rather than accessing individual eleements.
- Fixes the case where copyWithin is invoked on a TypedArray with a
detached buffer.
- Add tests to ensure that +/-Infinity (for all 3 parameters) is handled
  correctly by the
algorithm

The C++ version gets through the benchmark more than 25000 times as
quickly as the JS implementation.

BUG=v8:5925, v8:5929, v8:4648
R=cbruni@chromium.org, adamk@chromium.org, littledan@chromium.org

Review-Url: https://codereview.chromium.org/2697593002
Cr-Commit-Position: refs/heads/master@{#43213}

[modify] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/src/bootstrapper.cc
[modify] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/src/builtins/builtins.h
[modify] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/src/js/array.js
[modify] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/src/js/typedarray.js
[add] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/test/mjsunit/es6/regress/regress-5929-1.js
[modify] https://crrev.com/dc302c74be4183db1b84347afab893825c887add/test/mjsunit/es6/typedarray-copywithin.js

I ported %TypedArray%.prototype.fill to C++ locally using std::fill.

Benchmark (filling Float64Array(10000) with 1.0 for 1000 times):
JS fill : 0.021000000000000796 s
For-loop: 0.008000000000009777 s
C++ fill: 0.003999999999990678 s

C++ fill is only 2x faster than for-loop, still worth a look?
Updated benchmark (run with d8 --ignition-staging --turbo)

var initialLargeFloat64Array = new Array(10000);
for (var i = 0; i < 5000; ++i) {
  initialLargeFloat64Array[i] = i;
}
initialLargeFloat64Array = new Float64Array(initialLargeFloat64Array);
var largeFloat64Array;

function FillLarge() {
  largeFloat64Array.fill(1.0);
}

function FillJSLarge() {
  for (var i = 0; i < 10000; i++) {
    largeFloat64Array[i] = 1.0;
  }
}

function FillLargeSetup() {
  largeFloat64Array = new Float64Array(initialLargeFloat64Array);
}

function run(main) {
  var total = 0.0, start = 0.0, end = 0.0;
  for (var i = 0; i < 1000; i++) {
    FillLargeSetup();
    start = performance.now();
    main();
    end = performance.now();
    total += end - start;
  }
  print("Time: ", total);
}

run(FillLarge);
run(FillJSLarge);

Before:
Time:  55.032999999999745
Time:  14.608999999999938

After:
Time:  4.029000000000083
Time:  14.999999999999773
(Remove unrelated code, result unchanged)

var largeFloat64Array = new Float64Array(10000);

function FillLarge() {
  largeFloat64Array.fill(1.0);
}

function FillJSLarge() {
  for (var i = 0; i < 10000; i++) {
    largeFloat64Array[i] = 1.0;
  }
}

function run(main) {
  var total = 0.0, start = 0.0, end = 0.0;
  for (var i = 0; i < 1000; i++) {
    start = performance.now();
    main();
    end = performance.now();
    total += end - start;
  }
  print("Time: ", total);
}

run(FillLarge);
run(FillJSLarge);
Definitely worth having a fast and predictable fill.
Project Member Comment 17 by bugdroid1@chromium.org, Mar 7
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a9ace2989326eb51b2b1f4b9ab1185b050845944

commit a9ace2989326eb51b2b1f4b9ab1185b050845944
Author: cwhan.tunz <cwhan.tunz@gmail.com>
Date: Tue Mar 07 05:06:30 2017

[typedarrays] Move %TypedArray%.prototype.includes to C++ builtins

- Remove TypedArrayIncludes in src/js/typedarray.js
- Implement it to C++ using the IncludesValue implementation
  in ElementsAccessor

BUG=v8:5929

Review-Url: https://codereview.chromium.org/2732823002
Cr-Commit-Position: refs/heads/master@{#43625}

[modify] https://crrev.com/a9ace2989326eb51b2b1f4b9ab1185b050845944/src/bootstrapper.cc
[modify] https://crrev.com/a9ace2989326eb51b2b1f4b9ab1185b050845944/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/a9ace2989326eb51b2b1f4b9ab1185b050845944/src/builtins/builtins.h
[modify] https://crrev.com/a9ace2989326eb51b2b1f4b9ab1185b050845944/src/js/typedarray.js

Project Member Comment 18 by bugdroid1@chromium.org, Mar 13
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/b2efe57cdfb87698599c1135581b258ab6fd8aa5

commit b2efe57cdfb87698599c1135581b258ab6fd8aa5
Author: cwhan.tunz <cwhan.tunz@gmail.com>
Date: Mon Mar 13 09:40:09 2017

[typedarrays] Move %TypedArray%.prototype.indexOf to C++

- Remove TypedArrayIndexOf in src/js/typedarray.js
- Implement it to C++ using the IndexOfValue in ElementsAccessor
- Add buffer neutering check also for %TypedArray%.prototype.includes

BUG=v8:5929

Review-Url: https://codereview.chromium.org/2733193002
Cr-Commit-Position: refs/heads/master@{#43741}

[modify] https://crrev.com/b2efe57cdfb87698599c1135581b258ab6fd8aa5/src/bootstrapper.cc
[modify] https://crrev.com/b2efe57cdfb87698599c1135581b258ab6fd8aa5/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/b2efe57cdfb87698599c1135581b258ab6fd8aa5/src/builtins/builtins.h
[modify] https://crrev.com/b2efe57cdfb87698599c1135581b258ab6fd8aa5/src/js/typedarray.js

Labels: Priority-2
Project Member Comment 22 by bugdroid1@chromium.org, Mar 24
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a1f2239e0b7daff6bbf728d4f744ef6ee52f5299

commit a1f2239e0b7daff6bbf728d4f744ef6ee52f5299
Author: loorongjie <loorongjie@gmail.com>
Date: Fri Mar 24 22:43:35 2017

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}

[modify] https://crrev.com/a1f2239e0b7daff6bbf728d4f744ef6ee52f5299/src/elements.cc
[modify] https://crrev.com/a1f2239e0b7daff6bbf728d4f744ef6ee52f5299/test/mjsunit/es6/typedarray-fill.js

Project Member Comment 24 by bugdroid1@chromium.org, Apr 1
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c5c0765ad918d3606d7711d9dc5e727981ec8dcf

commit c5c0765ad918d3606d7711d9dc5e727981ec8dcf
Author: cwhan.tunz <cwhan.tunz@gmail.com>
Date: Sat Apr 01 16:46:10 2017

[typedarrays] Move %TypedArray%.prototype.slice to C++

- Implement %TypedArray%.prototype.slice to C++ builtins
- Remove TypedArraySlice in src/js/typedarray.js
- Implement TypedArraySpeciesCreate in builtins-typedarray.cc
- Implement TypedArrayCreate in builtins-typedarray.cc

BUG=v8:5929

Review-Url: https://codereview.chromium.org/2763473002
Cr-Commit-Position: refs/heads/master@{#44322}

[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/src/bootstrapper.cc
[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/src/builtins/builtins-definitions.h
[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/src/elements.cc
[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/src/elements.h
[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/src/js/typedarray.js
[modify] https://crrev.com/c5c0765ad918d3606d7711d9dc5e727981ec8dcf/test/mjsunit/es6/typedarray-slice.js

Project Member Comment 25 by bugdroid1@chromium.org, Apr 4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/2b1b32253bd4f0d6d97ae6c804d8e66eb1bfd07b

commit 2b1b32253bd4f0d6d97ae6c804d8e66eb1bfd07b
Author: Loo Rong Jie <loorongjie@gmail.com>
Date: Tue Apr 04 12:51:56 2017

[typedarray] ToNumber coercion is done only once for TA.p.fill

Update according to new spec change at
https://github.com/tc39/ecma262/pull/856

- Call ToNumber only once in BUILTIN
- Remove unused FillNumberSlowPath
- FillImpl assumes obj_value->IsNumber() is true
- Update test

Bug:v8:5929, chromium:702902 

Change-Id: Ic83e6754d043582955b81c76e68f95e1c6b7e901
Reviewed-on: https://chromium-review.googlesource.com/465646
Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44373}
[modify] https://crrev.com/2b1b32253bd4f0d6d97ae6c804d8e66eb1bfd07b/src/builtins/builtins-typedarray.cc
[modify] https://crrev.com/2b1b32253bd4f0d6d97ae6c804d8e66eb1bfd07b/src/elements.cc
[modify] https://crrev.com/2b1b32253bd4f0d6d97ae6c804d8e66eb1bfd07b/test/mjsunit/es6/typedarray-fill.js

Sign in to add a comment