New issue
Advanced search Search tips

Issue 884671 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

TypedArray.from is 100+ x too slow

Reported by maartenb...@gmail.com, Sep 17

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.81 Safari/537.36

Steps to reproduce the problem:
See https://jsperf.com/comparing-typed-array-casting  for a benchmark comparison. A good old look is about 100x faster than Float32Array.from(some_float_64_array).

What is the expected behavior?
It should be at least as fast.

What went wrong?
Float32Array.from is 100+ x slower than the manual loop

Did this work before? N/A 

Chrome version: 69.0.3497.81  Channel: n/a
OS Version: OS X 10.13.6
Flash Version:
 
Observed since TypedArray methods were shipped in Chrome 45 (325fbd0e243f6d43c977452651cf4be9a0dad727 via r335878).
Also observed in Canary.

Simplified test.html attached.
Expected: two small numbers like "5 5"
Observed: the first number is orders of magnitude larger like "200 5"
test.html
398 bytes View Download
Similar bugreport for firefox (15x slower):
https://bugzilla.mozilla.org/show_bug.cgi?id=1491813
Components: -Blink Blink>JavaScript
Labels: Needs-Triage-M69
Labels: Triaged-ET Target-71 M-71 FoundIn-71 OS-Linux OS-Windows
Status: Untriaged (was: Unconfirmed)
Able to reproduce the issue on Mac 10.13.3, Win-10 and Ubuntu 14.04 using chrome reported version #69.0.3497.81 and latest canary #71.0.3553.2.
This is a non-regression issue as it is observed from M60 old builds. 

Hence, marking it as untriaged to get more inputs from dev team.

Thanks...!!
Cc: hablich@chromium.org
Owner: petermarshall@chromium.org
Status: Assigned (was: Untriaged)
how much is this anticipated?
Hi. I have a candidate fix for this issue.

I'm having some firewall issues at the moment that are preventing me uploading the CL, hopefully I'll have those resolved tomorrow.
The other idiomatic way of doing this is just to use the typed array constructor. This is the same speed as the manual loop. The spec makes it easier for us because we don't have to interact with the iterator at all: https://tc39.github.io/ecma262/#sec-typedarray-typedarray

I'll review Daniel Shelton's fix (https://chromium-review.googlesource.com/c/v8/v8/+/1297312) - we can probably add this fast path, just need to be careful to follow the spec exactly and make sure we don't introduce any security issues.

var ar = new Float64Array(1e7);

[
  function() {
    new Float32Array(ar);
  },
  function() {
    var N = ar.length;
    var ar32 = new Float32Array(N);
    for(var i = 0; i < N; i++) {
      ar32[i] = ar[i];
    }
  }
].map(function(fn) {
  let start = Date.now();
  fn();
  console.log(Date.now() - start);
});
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/c7c0e110f5414e7debc83cb396a53c7a0b56abd2

commit c7c0e110f5414e7debc83cb396a53c7a0b56abd2
Author: Peter Marshall <petermarshall@chromium.org>
Date: Fri Oct 26 09:47:46 2018

[typedarray] Use fast path for Float32Array.from(float_64_array) and similar

Currently, because the source float_64_array has an iterator, it hits
the code in the "check_iterator" section of TypedArrayFrom which calls
IterableToList. This builds a temporary PACKED_ELEMENTS array (and boxes
all of the numeric values as HeapNumbers), then uses this as the source
array.

This patch checks if the source array is a TypedArray, and if the iterator
is the built-in one (where we know the iterator's behaviour). If both are
true then it bypasses the creation of this temporary array and uses the
original TypedArray as the source.

This allows it to take advantage of the existing fast code for copying one
typed array to another.

R=hablich@chromium.org, petermarshall@chromium.org

Bug:  chromium:884671 
Change-Id: I19a944c9d6d5d07699c7dc3ad7196fc871200b62
Reviewed-on: https://chromium-review.googlesource.com/c/1297312
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57022}
[modify] https://crrev.com/c7c0e110f5414e7debc83cb396a53c7a0b56abd2/AUTHORS
[modify] https://crrev.com/c7c0e110f5414e7debc83cb396a53c7a0b56abd2/src/builtins/builtins-typed-array-gen.cc
[add] https://crrev.com/c7c0e110f5414e7debc83cb396a53c7a0b56abd2/test/mjsunit/es6/typedarray-from-next-overridden.js
[add] https://crrev.com/c7c0e110f5414e7debc83cb396a53c7a0b56abd2/test/mjsunit/es6/typedarray-from-nonfunction-iterator.js

Status: Fixed (was: Assigned)
Thanks for reporting this issue and thanks to Daniel for the fix! This should reach canary in the next few days
Great seeing this fixed. Thanks all!

Sign in to add a comment