-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Speed up Lib.extendDeep (_extend) for arrays that have no object, array elements #732
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
Conversation
On a related note: based on animation needs, I made a function called deepExtendNoArrays that deep merges objects but simply transfers array references without copying at all. |
@rreusser thanks for mentioning, it would lead to higher efficiency and DRYness (my change keeps the existing behavior). What I don't know is if it's OK to use it in the place where it causes the slowdown - emitting an event here: https://github.com/plotly/plotly.js/blob/master/src/plot_api/plot_api.js#L2036. I suppose the extension is deep so that if a user modifies callback value contents, it has no effect. If it's not a concern, then it would be worth doing a PR with your 'deepExtendNoArrays' ahead of the other parts. ... on the other hand queue will be 0 length for most things anyway. |
Hmm… yeah, wasn't sure exactly what your need is, but we were talking and came to the conclusion (with maybe only an exception or two) that effectively any arrays in plotly json (i.e. traces—except for the array of traces themselves, of course) are data that can be transferred without copying. Perhaps a common function that copies traces/layouts without arrays and handles any edge cases would make sense at some point. |
@rreusser awesome, good to know! In the current case, an array of arrays is passed for extension, so probably the ... or actually, one level deeper as So it looks possible to directly use What's in this PR speeds up those cases where you can't make assumptions about which arrays need to be deep copied. This commit is compatible with |
Forgot to add one more thing that might help: it would be good in general to store large arrays as typed JS arrays. It's easy to test e.g. |
@monfera Agreed on locking down any corner cases where this might not be true. And hmm… agreed that typed arrays have benefits, but I might need extra convincing that the difference is worthwhile and actually the bottleneck before worrying that it might be premature optimization. 😄 |
Until then at least, I'll write a wrapper function to extend layouts and traces while simply moving array references with the goal of explicitly handling any corner cases where this transfer is not valid. (which, unfortunately, I think will be confined to my PR at the moment) |
@monfera Thanks for bringing this up! Have you done any benchmarking comparing More generally, I'm thinking that we may not need to extend the |
@etpinard yes, in the context of loading additional 1k points when we already have a lot of points. It's quite significant: Numbers: given 100k points, adding 1k new points takes:
The actual speedup is even more, because, clearly, this is not the only contribution to the overall runtime, so I think it's more like e.g. 40ms vs 300ms so around 10x or more speedup. |
Amazing. |
@etpinard if you want to deeply extend
|
This PR has the same effect on speed as the suggestion in #726 (comment)
The referred suggestion isn't obsoleted by this PR, because
deepExtend
, by its nature, is still expensive: with large datasets,Plotly.restyle
will still allocate and populate very large arrays (though much faster if this PR is merged).Similarly, this PR can be useful even outside the original speed problem, as it generally speeds up structure extension when large, simple arrays are present.