Issue metadata
Sign in to add a comment
|
A math operation is giving an NaN result -- but only when run a certain number of times within a loop.
Reported by
ven...@gmail.com,
Apr 9 2017
|
||||||||||||||||||||||
Issue description
Chrome Version : Version 59.0.3063.4 (Official Build) dev (64-bit)
URLs (if applicable) : n/a (it happens everywhere)
Other browsers tested:
Add OK or FAIL, along with the version, after other browsers where you
have tested this issue:
Safari: (not tested)
Chrome: FAIL
Chromium: FAIL
Firefox: OK
IE: OK
Edge: OK
What steps will reproduce the problem?
(1) Run the following code in the dev-tools console:
// if you decrease the loop-count by even one, the `NaN` will not appear
// (possibly it's platform-dependent, so try a higher number if you don't get the NaN detection + log right away)
for (var i = 0; i < 6086; i++) {
var params = [0,0.75,-.75,1];
var t = 0;
var result1 = params.map(function(param, i) {
return param * Math.pow(t, i);
});
var result2 = result1.reduce(function (pre, curr) {
return pre + curr;
});
if (Number.isNaN(result2)) {
console.log("NaN produced!");
break;
}
}
What is the expected result?
For the "NaN produced!" message to never be logged. Or at least, if does get logged, that it does so consistently. Instead, it only happens when the loop-count is above a certain number. (in my case, 6086)
What happens instead?
The "NaN produced!" message is logged, but only when above a certain loop-count threshold (6086), despite the code being functional/without-side-effect.
Please provide any additional information below. Attach a screenshot if
possible.
I've tried this multiple times, in multiple tabs, and in both Chrome and Chromium.
Regardless of which value is correct (0 or NaN), it should not give inconsistent results depending only on the loop-count.
You can find my original bug report (in a GitHub repo) here: https://github.com/recharts/react-smooth/issues/12
And here is the version of Chrome I'm using: "Version 59.0.3063.4 (Official Build) dev (64-bit)"
I was also able to reproduce the bug in Chromium "Version 59.0.3060.0 (Official Build) (64-bit)".
However, I was not able to reproduce the bug in Chromium "Version 59.0.3053.0 (Official Build) (64-bit)".
So the bug must have been introduced between versions "59.0.3053.0" and "59.0.3060.0".
,
Apr 9 2017
I tried your modified version, and get the NaN result with that as well. I've attached a recording of my running the code in dev-tools. (I've also uploaded it here: http://i.imgur.com/dyBdq5u.gifv )
,
Apr 9 2017
This seems to also be manifesting in the chart demos here: http://jsfiddle.net/ty4cn9p8/1/ This is the library by which I discovered the problem, and in the demos above, it manifests by causing the charts to often "disappear" while it's animating the chart from left to right. (I verified it was the same error by checking the console) This problem was not present in an earlier version of Chrome (when I first tried the library), but is present in the current version. (59.0.3060.0+)
,
Apr 9 2017
I just tried the code above on my laptop as well, and the NaN result occurs there as well. It also produces the NaN at the exact same loop-count: 5465 (well, that's one of the three results I've gotten) It also did *not* reproduce the error in the earlier version that I tried just before updating. (57.0.2987.133) To me, this verifies that it is a problem with Chrome, as opposed to a particular CPU -- or even a particular CPU manufacturer. (my desktop is an AMD FX 8320, and my laptop is an Intel Pentium B960) [they are both 64-bit, however]
,
Apr 10 2017
,
Apr 11 2017
Able to reproduce the issue on windows 7, Ubuntu 14.04 and Mac 10.12.3 using chrome version 59.0.3065.0.This is regression issue broken in M59. Please find the bisect information as below Narrow Bisect:: Good ::59.0.3055.0 --- (build revision 460255) Bad::59.0.3056.0 -- (build revision 460603) Change Log:: https://chromium.googlesource.com/chromium/src/+log/2cf54d71895488bec927412bc9634a51a2981d57..ebedc955fbd9688a074cf696227d7daea56d9f69 Possible suspect might be https://chromium.googlesource.com/v8/v8/+/0554e36be0a5406627370606b9791f1f9ec7abd3 bmeurer@ Could you please look into this issue if it is related to your change,else please help us in finding the appropriate owner for this issue. Thanks,
,
Apr 11 2017
Issue 707992 has been merged into this issue.
,
Apr 11 2017
Ok, the problem is with the way the Array builtins are implemented now, in this particular case with A.p.reduce. This is incompatible with the optimization in the JSCallReducer that fills up parameters for builtins up to formal_parameter_count. Specifically: [0].reduce(x => x) Here the JSCallReducer sees a call to the ArrayReduce builtin, with one parameter (the closure). The SharedFunctionInfo for the builtin says that it's internal_formal_parameter_count is 2, so TurboFan just appends one undefined. But this confuses the builtin, which peeks into the stack looking for an arguments adaptor frame. Now the builtin thinks you specify undefined as initial_value, and thus you produce undefined here instead of 0. I think the builtin should be declared with SharedFunctionInfo::kDontAdaptArgumentsSentinel instead, and don't peek into the stack. Not sure how easy it is to fix that. I'd be fine with just disabling the optimization in the JSCallReducer in the meantime. danno@ WDYT?
,
Apr 11 2017
Minimized repro:
-----------------------------------------
// Flags: --allow-natives-syntax
var a = [0];
function bar(x) { return x; }
function foo() { return a.reduce(bar); }
assertEquals(0, foo());
assertEquals(0, foo());
%OptimizeFunctionOnNextCall(foo);
assertEquals(0, foo());
-----------------------------------------
Probably also affects reduceRight, and maybe a couple of other builtins.
,
Apr 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d98dfd8b9b68635c3b974e1d91be414304dec35c commit d98dfd8b9b68635c3b974e1d91be414304dec35c Author: bmeurer <bmeurer@chromium.org> Date: Wed Apr 12 04:32:05 2017 Revert "[turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins." This reverts commit 9df5674bd53b4a262e72f45263df9e886842c269 because it is not compatible with the way that Array.prototype.reduceRight and Array.prototype.reduce deal with optional parameters at this point (i.e. parameters where the behavior is different depending on whether the parameter was skipped or undefined was passed). In general, it might be better to not adapt arguments for builtins with optional paramters, that are likely skipped, for example as in Object.create or Array.prototype.reduce. Since that will require arguments adaptor frames for normal calls, especially from baseline code. Instead it might make sense to use the variadic arguments support in the CodeStubAssembler instead to avoid the arguments adaptor in all cases (not only when called from TurboFan optimized code). BUG=v8:5267, chromium:709782 , chromium:707992 , chromium:708282 , chromium:708599 , chromium:709173 , chromium:709747 , chromium:707065 , chromium:710417 TBR=danno@chromium.org Review-Url: https://codereview.chromium.org/2817653002 Cr-Commit-Position: refs/heads/master@{#44593} [modify] https://crrev.com/d98dfd8b9b68635c3b974e1d91be414304dec35c/src/compiler/js-call-reducer.cc
,
Apr 12 2017
,
Apr 12 2017
,
Apr 12 2017
,
Apr 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/ddf03166c01372d8da269cfaa00188e3113465ce commit ddf03166c01372d8da269cfaa00188e3113465ce Author: Michael Hablich <hablich@chromium.org> Date: Thu Apr 13 12:23:05 2017 Merged: Revert "[turbofan] Avoid going through ArgumentsAdaptorTrampoline for CSA/C++ builtins." Revision: d98dfd8b9b68635c3b974e1d91be414304dec35c BUG= chromium:707065 , chromium:707992 , chromium:708282 , chromium:708599 , chromium:709173 , chromium:709747 , chromium:709782 , chromium:710417 ,v8:5267 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true TBR=bmeurer@chromium.org Change-Id: I2363c9012d7107e5e246d46bf6938bead642b486 Reviewed-on: https://chromium-review.googlesource.com/476351 Reviewed-by: Michael Hablich <hablich@chromium.org> Cr-Commit-Position: refs/branch-heads/5.9@{#4} Cr-Branched-From: fe9bb7e6e251159852770160cfb21dad3cf03523-refs/heads/5.9.211@{#1} Cr-Branched-From: 70ad23791a21c0dd7ecef8d4d8dd30ff6fc291f6-refs/heads/master@{#44591} [modify] https://crrev.com/ddf03166c01372d8da269cfaa00188e3113465ce/src/compiler/js-call-reducer.cc
,
Apr 18 2017
Verified the issue on Mac os 10.12.3 , ubuntu 14.04 and windows 7 using chrome dev M59 #59.0.3071.9 and issue is fixed. Steps followed to verify: 1. Launched chrome and on dev tools console , pasted the code given in comment #0 . 2. Observed no message in console. Attached screencast for reference. Adding TE-Verified labels. Thanks! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by ericg...@gmail.com
, Apr 9 2017Added 1 line to your code to print out all values of result2 on each iteration. Turns out it prints 6086 zeros for me, without NaNs. Code: for (var i = 0; i < 6086; i++) { var params = [0,0.75,-.75,1]; var t = 0; var result1 = params.map(function(param, i) { return param * Math.pow(t, i); }); var result2 = result1.reduce(function (pre, curr) { return pre + curr; }); console.log(result2); // new if (Number.isNaN(result2)) { console.log("NaN produced!"); break; } }