Check for typed array in minExtend

[jvandort]

Modified minExtend to check for both arrays and TypedArrays.

Currently, if a trace contains a typedArray, minExtend will treat it it as if it's not an array and will re-create the entire array as opposed to simply taking a slice of the array. To fix this, this change checks if the current value is an array in addition to checking if it is a TypedArray.

[jvandort]

Sorry for the failing PR. Looks like I gotta put a little more work into this

[etpinard]

Thanks for the PR!

Can I ask why you're patching this line? Does not considering typed arrays lead to any bugs? If so, can you provide a reproducible example? If it's only a performance gain, could you provide some before/after benchmarks?

[jvandort]

I've created a benchmark to show the problem's I'm having. The project I'm working on involves plotting many thousands of points, and due to performance problems, we decided to try to use TypedArrays instead of standard javascript arrays.

The issue we're running into seems to be related to rendering the legend. minExtend ends up re-creating the entire trace due to it not recognizing that the trace we pass in is a TypedArray.

Please see this codepen for more details:

https://codepen.io/jvandort/pen/OJJyQpq

Open up chrome dev tools, click on the performance tab, and begin recording. Then, click 'Reload' once, and then again.

The first time, Plotly.React is called with a standard javascript array. The second time, it is called with a TypedArray. You can see that minExtend takes up the majority of the processing time for the second round (with TypedArray) since it ends up re-creating the entire array.

I've also attached a screenshot of the chrome dev tools performance tab while I ran the procedure that I posted above.

EDIT: With the fix applied:

EDIT: Here, in /src/components/color.js, there also seems to be a similar issue. This code is looking for legacy colors, and is only looking for keys with strings that end in 'color' or 'colorscale'. It will also recurse all the way though an object looking for these keys. It checks if the object is an Array, but does not check if it is a TypedArray. So, it ends up iterating over the input trace even though there is no reason for it to.

plotly.js/src/components/color/index.js

Line 125 in df35b38

if(!Array.isArray(el0) && el0 && typeof el0 === 'object') {

You can see evidence of that in my second screenshot where you can visibly see 'color.clean' in the flame graph for the second run of the code (with TypedArray), but not the first.

[etpinard]

Wow. Thanks so much for spotting that @jtvd78 !

You're absolutely right: something is up in our legend code and your patch does seem to resolve most of the slow perf.

Let me see if I can identify why some of our tests are failing.

[etpinard]

Ok, I got the tests to pass on this branch:

https://github.com/plotly/plotly.js/compare/pr-4268-etpinard

---

Turns out that your patch did fix a bug:

        Plotly.newPlot(gd, [{
            x: new Float32Array([1, 2, 3]),
            y: new Float32Array([1, 2, 1]),
            marker: {
                size: new Float32Array([40, 30, 20]),
                color: new Float32Array([20, 30, 10]),
                cmin: 10,
                cmax: 30,
                colorscale: [
                    [0, 'rgb(255, 0, 0)'],
                    [0.5, 'rgb(0, 255, 0)'],
                    [1, 'rgb(0, 0, 255)']
                ]
            }
        }], {
          showlegend: true
        })

currently renders as

and now off your branch

with the correct legend item marker color 🎉 so that scatter_test.js should now be updated ✅

Next, .slice isn't supported for typed arrays on some (old) browsers (like the nw.js version we used for our image tests). Using .subarray for typed arrays in Lib.minExtend makes the image tests pass again.

---

@jtvd78 feel free to pull down my branch and cherry-pick my commits or make the changes yourself.

If you haven't replied by next Tuesday, I'll merge my pr-4268-etpinard branch so that it is released as part of v1.50.1.

[jvandort]

Not sure what's going on here. I've updated my branch with your changes but it seems there are still failing tests

EDIT: I've got another similar change I want to make (I mentioned it in my previous post), but this time I'm actually planning on testing it first. I've got repo cloned, but I can't seem to run npm run test-jasmine without getting failing tests, even on master. Am I doing something wrong? (I followed the guide here: https://github.com/plotly/plotly.js/blob/master/CONTRIBUTING.md)

Either way, here are the changes to color.clean that fixes a problem similar to this PR.

https://github.com/jtvd78/plotly.js/commit/e9157f1eb65575b9e3ad6eb8912e02db6e97f8ea

[etpinard]

@jtvd78 the failures on CI were just intermittent. After rebuilding the tests are now ✅

Thanks very much again for this PR! Merging!!

---

but I can't seem to run npm run test-jasmine without getting failing tests, even on master. Am I doing something wrong?

Yeah, that can happen unfortunately. The tests are tuned for the hardware and Chrome version found on our CI. For 99% of our tests that doesn't matter, but for some tests that depend on assertions in pixel space and font rendering we have noticed some machine-to-machine discrepancies.