New issue
Advanced search Search tips

Issue 673137 link

Starred by 1 user
Status: Duplicate
Merged: issue 666947
Owner:
ishell@chromium.org
Closed: Dec 2016
Cc:
bmeu...@chromium.org
hablich@chromium.org
chrome-perf-bugs@google.com
verwa...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Performance issue: one JS function suddenly takes ~60% of runtime in BBC Micro Emulator

Reported by m...@godbolt.org, Dec 11 2016 
Chrome Version       : 55.0.2883.75 (Official Build) (64-bit)
Operating System and Version: Linux Ubuntu 16.10
URLs (if applicable) : https://bbc.godbolt.org/

Description of performance problem:

Since upgrading to Chrome 55, the emulator I wrote on bbc.godbolt.org (source on GitHub at https://github.com/mattgodbolt/jsbeeb) has slowed down by a factor of two.

Profiling reveals that 60% of the time is now spent in a single function: "video.clearFb' which is a simple loop to write 0xff000000 to a set of Uint32Array locations.

A similar function nearby (which writes values looked up from a table instead of a constant) is much faster. In fact, if one replaces the call to clearFb with the equivalent call to "blitFb", things speed up immensely. Much more detail is filed on a bug on jsbeeb at https://github.com/mattgodbolt/jsbeeb/issues/138 where I describe some things I tried to see if I could speed it up.

Given the nature of the slowdown I wonder if there's something unusual going on with the particular way this function is written, although I tried rephrasing it several ways.

The attached trace is of me running the emulator with an intense cpu profile going (which just runs instructions). This can be reproduced by getting the JS console up and typing 'profileCpu()' in the console.

I'd be delighted to help with any other information needed to track this down. Apologies if I've overlooked something.
trace_slow-in-chrome-55.json.gz
4.6 MB Download

Comment 1 by m...@godbolt.org, Dec 11 2016 

Seems this bug has been fixed in Chrome 56.

A cut-down repro is at http://bbc.godbolt.org/beta/perf-test.html which runs at ~1000ms for Chrome 55 and ~31ms in Chromes 54 and 56+.

The repro code is:

function tst(fb32, destOffset, numPixels) {
    var black = 0xFF000000;
    while (numPixels--) {
        fb32[destOffset++] = black;
    }
}
var fb32 = new Uint32Array(1024);
var reps = 500000;
var start = Date.now();
for (var i = 0; i < reps; ++i)
    tst(fb32, 0, 16);
var end = Date.now();
var result = "Took " + (end - start) + "ms to do " + reps + " 16 32-bit clears";
console.log(result);

Comment 2 by bmeu...@chromium.org, Dec 11 2016

Cc: bmeu...@chromium.org
Owner: ishell@chromium.org
Status: Assigned (was: Unconfirmed)
Looks like the keyed IC regression.

Comment 3 by bmeu...@chromium.org, Dec 11 2016

Cc: hablich@chromium.org
Components: Blink>JavaScript>Runtime
Labels: M-55 Arch-All OS-All
I have a strong suspicion that this is a duplicate of https://bugs.chromium.org/p/chromium/issues/detail?id=666947. The perf test is always storing a HeapNumber, since we cannot represent 0xFF000000 as Smi in boxed form, which was the core issue that caused the vellamo regression.

ishell@ please verify and request stable merge if it's the same. I think the patch had decent coverage now and it's not too risky IMHO.

Comment 4 by hablich@chromium.org, Dec 12 2016

Mergedinto: 666947
Status: Duplicate (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 12 2016

Labels: merge-merged-5.5
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fc0a0d7699c039df53961d47291981b151aba44f

commit fc0a0d7699c039df53961d47291981b151aba44f
Author: ishell@chromium.org <ishell@chromium.org>
Date: Mon Dec 12 08:43:20 2016

Merged: [ic] Prevent KeyedStoreIC from being generic when storing doubles to integer typed arrays.

Revision: c819616376b6835b61ff4f9e2667ba94ab25fe7f

BUG= chromium:666947 ,  chromium:673137 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=bmeurer@chromium.org

Review-Url: https://codereview.chromium.org/2564323003 .
Cr-Commit-Position: refs/branch-heads/5.5@{#66}
Cr-Branched-From: 3cbd5838bd8376103daa45d69dade929ee4e0092-refs/heads/5.5.372@{#1}
Cr-Branched-From: b3c8b0ce2c9af0528837d8309625118d4096553b-refs/heads/master@{#40015}

[modify] https://crrev.com/fc0a0d7699c039df53961d47291981b151aba44f/src/code-stub-assembler.cc
[modify] https://crrev.com/fc0a0d7699c039df53961d47291981b151aba44f/src/code-stub-assembler.h
[modify] https://crrev.com/fc0a0d7699c039df53961d47291981b151aba44f/src/compiler/code-assembler.cc
[modify] https://crrev.com/fc0a0d7699c039df53961d47291981b151aba44f/src/compiler/code-assembler.h
►

Sign in to add a comment