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.

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

fallbacks for IE9 #1297

Merged
merged 6 commits into from Jan 13, 2017
Merged

fallbacks for IE9 #1297

merged 6 commits into from Jan 13, 2017

Conversation

alexcjohnson
Copy link
Contributor

Within heatmap/contour map it turns out to be better to just use a bare array than to try and use the typedarray polyfill - the standard implementation there is limited to 1e5 points in the array (which is tiny for our purposes) with a note that anything bigger bogs down unacceptably. We don't need validation or anything, and were just using it for speed.

With this patch, plotly.js loads in IE9 (if you bundle it without any of our webGL modules, including 3d, gl2d, and mapbox) and all the other trace types and components except our d3 maps work as expected.

cc @etpinard @chriddyp

but it works this way with the polyfill too.
@chriddyp
Copy link
Member

🙇

@@ -337,7 +337,14 @@ function plotOne(gd, plotinfo, cd) {

if(zsmooth) { // best or fast, works fastest with imageData
var pxIndex = 0,
pixels;

try {
pixels = new Uint8Array(imageWidth * imageHeight * 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Can we polyfill this instead of relying on a slow try-catch?

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake. I jumped the 🔫 and didn't read your PR description. try-catch do that to me, I find them particularly irritating.

the standard implementation there is limited to 1e5 points in the array (which is tiny for our purposes) with a note that anything bigger bogs down unacceptably.

Point taken. But, I'd call slowing down performance to support IE9 also unacceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought you might say that... do we have data on how bad this issue is though? This is two try blocks per trace (which won't necessarily both fail together), no loops, is that really worrisome? Do you see a robust alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some quick testing in Chrome and FF shows a negligible (~ns) impact of a try that does not catch, but a substantial impact (~µs) for a try that fails and goes on to catch. Assuming similar behavior in other browsers, that would mean modern browsers (that do not need the fallback) will not be hindered, and those that DO need the fallback will be delayed by at most a few tens of µs.

So I think we're OK here, but yeah, this clearly shows that try/catch should never be used within a loop, unless the catch will be exceedingly rare!

function timeit(f, v, n) {
    var t0 = performance.now();
    for(i = 0; i < n; i++) f(v);
    var t1 = performance.now();
    return (t1 - t0) / n;
}

// some representative numbers from trying many times

timeit(function(v) {if(v) throw new Error('boo'); return 0;}, 0, 10000000)
0.000008333999998867512 // chrome: 8.3 ns with no try/catch
0.00044195000000001164 // FF: 442 ns with no try/catch (why so much slower???)

timeit(function(v) {try{if(v) throw new Error('boo');} catch(e) {return 1;} return 0;}, 0, 10000000)
0.000009642499999701976 // chrome: 9.6 ns with try but no catch
0.0004438450000000885 // FF: 444 ns with try but no catch

timeit(function(v) {try{if(v) throw new Error('boo');} catch(e) {return 1;} return 0;}, 1, 1000000)
0.007863459999993444 // chrome: 7.9 µs hitting the catch
0.003594469999999972 // FF: 3.6 µs hitting the catch

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 checking!

I'm curious. Would a fallback be faster than try-catch:

var pixels = (new Uint8Array || new Array)(imageWidth * imageHeight * 4);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(new Uint8Array || new Array)(imageWidth * imageHeight * 4) would fail most of the time in old browsers:

  • If you don't include the typedarray polyfill, then Uint8Array is not just undefined, it's a bad symbol. I suppose we could get around that by saying window.Uint8Array but...
  • If you do include the polyfill, but imageWidth * imageHeight * 4 > 1e5 then the Uint8Array constructor will execute and throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could get around that by saying window.Uint8Array but...

Yeah, that's what I meant to write.

If you do include the polyfill, but imageWidth * imageHeight * 4 > 1e5 then the Uint8Array constructor will execute and throw an error.

Good point here but .... what if we search for another polyfill that handles > 1e5 arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if we search for another polyfill that handles > 1e5 arrays?

In order to really make something that behaves like a typed array, you need to add a whole lot of machinery, so no matter what you do it's going to be much slower than a bare array. That might be justified if you truly need to enforce the type restrictions, but we don't, we already have constructed our colors to be integers 0-255. So this overhead is simply harmful to us, far better to use a bare Array than any conceivable polyfill.

So then the question is, is there ever a situation where we truly need the polyfill? Seems like no, right? Because any system that supports WebGL will already support typed arrays, and nothing else is affected by this.

And then I guess we could ask if there's a direct way to tell if we have real typed arrays vs polyfills. But honestly, I think based on my perf testing above, for the modern case - where we don't hit either of those catch blocks - this is actually the fastest possible code; short of doing the feature detection at compile time and completely swapping out the plot routine 😱 we're not going to get less overhead than this. And for the fallback case we're only talking about a few tens of µs delay, hardly problematic for folks whose browsing experience is likely subject to far worse restrictions and bugs by now.

Copy link
Contributor

@etpinard etpinard Jan 13, 2017

Choose a reason for hiding this comment

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

Ok. I convinced me. 👍

But, we should agree on a way to test these catch blocks in #1299 before 💃 .

@etpinard
Copy link
Contributor

Possible way to test IE9 stuff -> #1299

- in bundle_tests/
- mock IE9 env before loading plotly.js + test bundle
Test IE9 by mocking window globals
@etpinard
Copy link
Contributor

💃 once test ✅

@alexcjohnson alexcjohnson merged commit 3b85f7f into master Jan 13, 2017
@alexcjohnson alexcjohnson deleted the ie9-fix branch January 13, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants