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

Jump to bottom

fallbacks for IE9 #1297

Merged
merged 6 commits into from
Jan 13, 2017
Merged

fallbacks for IE9 #1297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e81eca
fallbacks for ie9 are better than typedarray polyfill
alexcjohnson Jan 12, 2017
c0490d6
try adding 'ie9' tests
etpinard Jan 13, 2017
4ce9df0
take 2 on ie9 test
etpinard Jan 13, 2017
7cb691e
AJ proof ie9 bundle test
etpinard Jan 13, 2017
2b1c0fe
Revert "try adding 'ie9' tests"
etpinard Jan 13, 2017
675103c
Merge pull request #1299 from plotly/test-ie9
etpinard Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/lib/is_array.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
/**
* Return true for arrays, whether they're untyped or not.
*/

// IE9 fallback
var ab = (typeof ArrayBuffer === 'undefined' || !ArrayBuffer.isView) ?
{isView: function() { return false; }} :
ArrayBuffer;

module.exports = function isArray(a) {
return Array.isArray(a) || ArrayBuffer.isView(a);
return Array.isArray(a) || ab.isView(a);
};
19 changes: 18 additions & 1 deletion src/traces/heatmap/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Collaborator 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
Collaborator 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
Collaborator 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
Collaborator 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 •
edited

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

}
catch(e) {
pixels = new Array(imageWidth * imageHeight * 4);
}

if(zsmooth === 'best') {
var xPixArray = new Array(x.length),
Expand Down Expand Up @@ -379,7 +386,17 @@ function plotOne(gd, plotinfo, cd) {
}

var imageData = context.createImageData(imageWidth, imageHeight);
imageData.data.set(pixels);
try {
imageData.data.set(pixels);
}
catch(e) {
var pxArray = imageData.data,
dlen = pxArray.length;
for(j = 0; j < dlen; j ++) {
pxArray[j] = pixels[j];
}
}

context.putImageData(imageData, 0, 0);
} else { // zsmooth = false -> filling potentially large bricks works fastest with fillRect
for(j = 0; j < m; j++) {
Expand Down
8 changes: 8 additions & 0 deletions test/jasmine/assets/ie9_mock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
delete window.Promise;

delete window.ArrayBuffer;
delete window.Uint8Array;
delete window.Float32Array;
delete window.Float64Array;
delete window.Int16Array;
delete window.Int32Array;
42 changes: 42 additions & 0 deletions test/jasmine/bundle_tests/ie9_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
var Plotly = require('@lib/core');

Plotly.register([
require('@lib/bar'),
require('@lib/box'),
require('@lib/heatmap'),
require('@lib/histogram'),
require('@lib/histogram2d'),
require('@lib/histogram2dcontour'),
require('@lib/pie'),
require('@lib/contour'),
require('@lib/scatterternary'),
require('@lib/ohlc'),
require('@lib/candlestick')
]);

var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');

describe('Bundle with IE9 supported trace types:', function() {

afterEach(destroyGraphDiv);

it(' check that ie9_mock.js did its job', function() {
expect(function() { return ArrayBuffer; })
.toThrow(new ReferenceError('ArrayBuffer is not defined'));
expect(function() { return Uint8Array; })
.toThrow(new ReferenceError('Uint8Array is not defined'));
});

it('heatmaps with smoothing should work', function(done) {
var gd = createGraphDiv();
var data = [{
type: 'heatmap',
z: [[1, 2, 3], [2, 1, 2]],
zsmooth: 'best'
}];

Plotly.plot(gd, data).then(done);
});

});
13 changes: 13 additions & 0 deletions test/jasmine/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ var arg = process.argv[4];
var testFileGlob = arg ? arg : 'tests/*_test.js';
var isSingleSuiteRun = (arg && arg.indexOf('bundle_tests/') === -1);
var isRequireJSTest = (arg && arg.indexOf('bundle_tests/requirejs') !== -1);
var isIE9Test = (arg && arg.indexOf('bundle_tests/ie9') !== -1);

var pathToMain = '../../lib/index.js';
var pathToJQuery = 'assets/jquery-1.8.3.min.js';
Expand Down Expand Up @@ -127,6 +128,18 @@ else if(isRequireJSTest) {
testFileGlob
];
}
else if(isIE9Test) {
// load ie9_mock.js before plotly.js+test bundle
// to catch reference errors that could occur
// when plotly.js is first loaded.

func.defaultConfig.files = [
'./assets/ie9_mock.js',
testFileGlob
];

func.defaultConfig.preprocessors[testFileGlob] = ['browserify'];
}
else {
func.defaultConfig.files = [
pathToJQuery,
Expand Down