New issue
Advanced search Search tips

Issue 703226 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 698746


Participants' hotlists:
I-TF-Launch


Sign in to add a comment

Chromium debug build is very slow running Javascript

Project Member Reported by rtoy@chromium.org, Mar 20 2017

Issue description

Chrome Version: 59.0.3047.0 (Developer Build) (64-bit)
OS: (e.g. Win7, OSX 10.9.5, etc...)

What steps will reproduce the problem?
(1) Do a release and debug build of Chromium
(2) Load third_party/WebKit/LayoutTests/webaudio/AudioBufferSource/sample-accurate-scheduling.html
(3) See how long it takes to run the test in both release and debug builds.
(4) In the debug build add the command-ling flag --enable-features=V8NoTurbo and compare the runtimes again.

What is the expected result?

The debug build without and without Turbofan should be roughly the same, not 3 times longer with Turbofan.

What happens instead?

On a Z620, a debug build takes about 20-25 sec to run the test,  With Turbofan disabled, it takes about 8 sec.

This causes the test timeout when run locally.  (Not sure why this isn't seen on the buildbots).  It could also be the cause of MSAN bots reporting flaky tests recently, which never used to happen.



 
Cc: mvstan...@chromium.org rmcilroy@chromium.org hablich@chromium.org
Components: Blink>JavaScript>Compiler
Blocking: 698746
Status: Available (was: Untriaged)
Owner: verwa...@chromium.org
Status: Started (was: Available)
Status: Fixed (was: Started)
This is due to the function window.Audit.prototype.beEqualToArray which internally does: 

 662       for (let index in this._actual) {
 663         if (this._actual[index] !== this._expected[index])
 664           errorIndices.push(index);
 665       }

And that's called on typed arrays. For/In has to convert keys (index) to strings (typeof index == "string"). I had changed how expected[index] and actual[index] dealt with index being strings, which caused all those indicates (in string shape) to be internalized.

I fixed this, which improves the test from ~25 to ~4 seconds.

If you additionally change it to the following, it runs in ~1 second:

       let actual = this._actual;
       let expected = this._expected;
       for (let index = 0; index < actual.length; index++) {
         if (actual[index] != expected[index])
           errorIndices.push(index);
       }

verwaest@ 

Can I ask where this is fixed?
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 23 2017

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

commit 20a803fd3c82fe6f644db4668f3b61f5e53b5958
Author: Toon Verwaest <verwaest@chromium.org>
Date: Thu Mar 23 16:51:38 2017

[runtime] Make sure we don't internalize string-encoded indices on KeyedGetProperty

BUG= chromium:703226 

Change-Id: I2232d4a721beb35478066b25143b9635bcc6b238
Reviewed-on: https://chromium-review.googlesource.com/458429
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44073}
[modify] https://crrev.com/20a803fd3c82fe6f644db4668f3b61f5e53b5958/src/runtime/runtime-object.cc

Comment 7 by rtoy@chromium.org, Mar 23 2017

c#4 sounds like a missed optimization opportunity for the compiler (converting the for-in-array to for index loop).  (I don't know anything about compilers, so it might not really be possible or very hard to do.)
Indeed, that optimization is pretty hard to do. And the alternative will always be faster in unoptimized code.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 24 2017

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

commit 97300c5c38f775bdec197909e4140ce649c5713a
Author: Toon Verwaest <verwaest@chromium.org>
Date: Fri Mar 24 14:05:29 2017

[runtime] Immediately set the array-index hash in Uint32ToString and convert to string in ForInPrepare

This speeds up for/in over arrays. E.g.,:

function f(a) {
  for (let i in a) {
    if (a[i] != a[i]) print(false);
  }
}
var a = new Array(10000000);
a.fill(1);
f(a);

runs 3x faster after the change.

BUG= chromium:703226 

Change-Id: Iabc5e931d985a03f89440cd702b2feb3eb9f5c18
Reviewed-on: https://chromium-review.googlesource.com/459538
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/master@{#44108}
[modify] https://crrev.com/97300c5c38f775bdec197909e4140ce649c5713a/src/builtins/builtins-forin-gen.cc
[modify] https://crrev.com/97300c5c38f775bdec197909e4140ce649c5713a/src/factory.h
[modify] https://crrev.com/97300c5c38f775bdec197909e4140ce649c5713a/src/objects.cc
[modify] https://crrev.com/97300c5c38f775bdec197909e4140ce649c5713a/src/runtime/runtime-forin.cc

I tweaked the perf for for-in over array, and HasProperty for typed arrays. Combined this makes

function f(a) {
  for (let i in a) {
    if (a[i] != a[i]) print(false);
  }
}
var a = new Uint32Array(10000000);
f(a);

run on my desktop in ~920ms down from >3s.

Using 

function f(a) {
  for (let i = 0; i < a.length; i++) {
    if (a[i] != a[i]) print(false);
  }
}
var a = new Uint32Array(10000000);
f(a);

is still much faster at ~40ms though :)

Sign in to add a comment