Remove Internet Explorer-specific Number.isNaN polyfill

[adroitwhiz]

Proposed Changes

This change removes the polyfill for the Number.isNaN() method, which is needed for Internet Explorer support.

Reason for Changes

Scratch doesn't support Internet Explorer.

[thisandagain]

Resolves #2157

[kchadha]

@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...

[towerofnix]

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?)

[mzgoddard]

@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.)

[mzgoddard]

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.

[adroitwhiz]

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?

[adroitwhiz]

@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.

[mzgoddard]

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

[kchadha]

LGTM! I'm sorry this PR took so long to review/approve. Thank you for your patience!