Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Internet Explorer-specific Number.isNaN polyfill #2146

Merged
merged 1 commit into from
Sep 17, 2019
Merged

Remove Internet Explorer-specific Number.isNaN polyfill #2146

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions src/util/cast.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
const Color = require('../util/color');

/**
* Store and possibly polyfill Number.isNaN. Number.isNaN can save time over
* self.isNaN by not coercing its input. We need to polyfill it to support
* Internet Explorer.
* @const
*/
const _NumberIsNaN = Number.isNaN || isNaN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mzgoddard, do you know why we added this change? I thought I might have commented on it in the original PR, but I think my comment was probably about something else...

Copy link
Contributor

@towerofnix towerofnix May 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relevant discussion is here: https://github.com/LLK/scratch-vm/pull/1709/files/5b10d41ba32f321a94aac4a6fb39656cfca4a2f0#diff-70e40f35658352341bf64fe5c0c23303

Seems it was swapped from isNaN to Number.isNaN for performance benefit, and there was some discussion about that. The only mention of IE is from mzgoddard:

I made the change (with back compat support in mind for Internet Explorer which does not provide Number.isNaN, in which case self.isNaN is a safe backup) and recorded some new numbers.

(edit: actually there's presumably some other discussion I can't find on the PR, per your reviews?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kchadha @towerofnix Yeah I don't recall the reason for the polyfill addition. I think it was because Number.isNaN was a new "idea" for the Scratch repos. self.isNaN is used the rest of the time. (We should use Number.isNaN instead though.)


/**
* @fileoverview
* Utilities for casting and comparing Scratch data-types.
Expand All @@ -33,13 +25,13 @@ class Cast {
if (typeof value === 'number') {
// Scratch treats NaN as 0, when needed as a number.
// E.g., 0 + NaN -> 0.
if (_NumberIsNaN(value)) {
if (Number.isNaN(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep const _NumberIsNaN = and use if (_NumberIsNaN) in the file. I added the polyfill for safety. But I originally had the constant to store the lookup. toNumber is simply used so much that this detail has a tiny but regular performance impact.

Copy link
Contributor Author

@adroitwhiz adroitwhiz May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some (admittedly rough) benchmarks, and it looks like there's no difference at all in Chrome, and in Firefox, caching Number.isNaN is actually... slower.

const _numberIsNaN = Number.isNaN;

function isnan1() {
	const nums = [];
	const startTime = performance.now();
	for (let i = 0; i < 10000000; i++) {
		nums.push(Number.isNaN(i));
	}
	console.log(performance.now() - startTime);
}

function isnan2() {
	const nums = [];
	const startTime = performance.now();
	for (let i = 0; i < 10000000; i++) {
		nums.push(_numberIsNaN(i));
	}
	console.log(performance.now() - startTime);
}

Can you reproduce these results with this benchmark code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mzgoddard Can you follow up on this? It's entirely possible my benchmark boilerplate is faulty, but it seems like the negative performance impact on Firefox points to removing the polyfill.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience.

@kchadha If I am the block on this I don't want to block this change longer. The detail on this has a small impact. With that said, here are some bits I want to state.

Micro-benching the difference between Number.isNaN and _numberIsNaN we need to measure the time in toNumber before and during isNaN is called. Engines will treat calling a function once inside another function differently than a function N times in another.

Your current benchmark snippet measures a function calling isNaN 10,000,000 times. But toNumber doesn't call isNaN 10,000,000 times, it is only called once.

Also, I don't think we can use push in the benchmark, because it modifies the array on every call. On some calls, since all push calls in the for loop add 10,000,000 booleans to the array, will allocate more memory to add those boolean references.

Here is an example of a benchmark I create that I believe covers the given needs:
https://esbench.com/bench/5d30d4ff4cd7e6009ef626b5

I ran my example benchmark through current chrome, firefox and safari on my development machine and saw these results like these over multiple executions.

result style Number.isNaN (ops/s) ± samples _numberIsNaN (ops/s) ± samples difference (ops/s) change
chrome array setter 862,034,379 0.17% 861,783,310 0.56% -251,069 -0.03%
return 1,147,573,444 0.41% 1,176,398,439 0.53% 28,824,995 2.51%
firefox array setter 925,157,247 3.73% 925,593,183 3.34% 435,936 0.05%
return 940,986,646 3.64% 958,097,362 3.63% 17,110,716 1.82%
safari array setter 1,162,073,129 0.39% 1,193,078,945 1.03% 31,005,816 2.67%
return 1,392,292,235 0.40% 1,377,016,404 1.11% -15,275,831 -1.10%

I tested a version with array operations and one with normal return syntax. The array setter version doesn't seem to have a change for chrome and firefox, but does for safari. The return syntax sees a positive change in chrome and firefox but a negative one in safari.

Some safari runs saw no change with the return syntax. It's hard to prove but I think the array setter syntax is sometimes hitting an optimization heuristic on safari with Number.isNaN. But the return syntax is not hitting that heuristic for _numberIsNaN.

With micro benchmarks as one tool, I like to depend more on scratch-vm's benchmark and chrome, firefox, and safari's javascript profilers. The micro benchmark doesn't reflect the different decisions js engines make on scratch-vm's whole code base.

I ran the two versions with Number.isNaN or _numberIsNaN on #2196 with project 89811578.

Number.isNaN (b/s) stdev _numberIsNaN (b/s) stdev difference change
2910545 17009 3005299 46038 94754 3.26%

Though that feels a bit cherry-picked. Controlling for the variables around running the VM on details this small is pretty hard. Memory has a big effect on this scale. Opening a tab for the same vm and project after running a version of that vm in another tab a few times is how I've found I can best control for a lot of stuff. So the above comparison is the average of runs 1 to 5 in a tab, after running the same vm version 10 other times in another tab.

Runs 1 to 15 data
run 1 run 2 run 3 run 4 run 5
average b/s stdev blocks seconds b/s blocks seconds b/s blocks seconds b/s blocks seconds b/s blocks seconds b/s
_numberIsNaN 1st 5 2780467 43674 11915427 4.526 2632662 13205726 4.497 2936563 12429016 4.52 2749782 12537071 4.51 2779838 12646548 4.511 2803491
2nd 5 2889635 15288 13001151 4.499 2889787 13081828 4.472 2925275 12768474 4.515 2828012 13002927 4.457 2917417 12957032 4.487 2887683
3rd w/ new tab 3005299 46038 13352466 4.522 2952779 14324232 4.482 3195946 13256038 4.523 2930807 13095817 4.489 2917313 13612209 4.493 3029648
Number.isNaN 1st 5 2900260 35831 12574898 4.463 2817589 12850257 4.519 2843606 13010610 4.513 2882918 13140442 4.516 2909752 13710415 4.499 3047436
2nd 5 2917416 26605 12883189 4.519 2850894 12905901 4.499 2868615 13056751 4.508 2896351 13557918 4.505 3009527 13318735 4.497 2961693
3rd w/ new tab 2910545 17009 13268441 4.459 2975654 13192035 4.509 2925712 13091194 4.508 2903992 12991558 4.519 2874875 12998030 4.525 2872493

return 0;
}
return value;
}
const n = Number(value);
if (_NumberIsNaN(n)) {
if (Number.isNaN(n)) {
// Scratch treats NaN as 0, when needed as a number.
// E.g., 0 + NaN -> 0.
return 0;
Expand Down