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

Ensure that extendDeepNoArrays includes typed arrays by reference #866

Merged
merged 4 commits into from Aug 19, 2016

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Aug 19, 2016

These changes ensure that extendDeepNoArrays includes by reference (i.e. does NOT deep-copy) typed arrays. Initially it looked as though the definition of isArray needs to be changed to

var isArray = function(a) {
    return ArrayBuffer.isView(a) || Array.isArray(a);
};

because Array.isArray returns false for typed arrays.

But it turned out that even the plain extendDeep already didn't deep-copy TYPED arrays. Therefore it was sufficient to add test coverage to ensure that extendDeepNoArrays doesn't deep-copy the typed arrays.

An additional commit locks in the current, untested extendDeep behavior with typed arrays. However it's contentious because maybe the reason for this extendDeep behavior is that perhaps the plotly.js doesn't use expect typed arrays (expects UNtyped arrays expressly or by implication). There are some good arguments for the benefits of the current extendDeep behavior when it comes to handling user-supplied typed-array input - e.g. typed arrays generally indicate performance awareness, therefore the user can reasonably expect structure sharing internally. It would make sense to treat typed arrays as atomic values, which is the current extendDeep behavior. It means that for performance critical code sections that would use typed array, the plotly.js contributor has to bear in mind that typed arrays are included by reference.

If the second commit isn't timely, I'm glad to reset it, but even in that case it's useful to ensure that extendDeepNoArrays doesn't deep copy typed arrays (first commit) as there's a WebGL plot performance improvement that hinges on it.

@etpinard
Copy link
Contributor

@monfera Thanks for looking into this!

But, why exactly do we need to worry about this? From what I know, the only typed arrays that we keep in memory are linked to gl trace objects nested in fullLayout.scene._scene and fullLayout._plots.xy._scene2d which shouldn't need to be extended or copied I don't think.

@@ -1,3 +1,5 @@
/*global Float32Array, Float64Array, Int16Array, Int32Array */
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add these to the test/jasmine/.eslintrc file similar to the src/.eslintrc

@@ -5,6 +5,10 @@
"jasmine": true
},
"globals": {
"Promise": true
"Promise": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you!

@etpinard
Copy link
Contributor

Great PR. Thanks1

@etpinard etpinard merged commit 3a343e2 into plotly:master Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants