-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Accept objects for encoded typedarrays in data_array valType #5230
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
- move decoding step to the start of calc data
@jonmmease are you interested to review & possibly work on this PR? |
I'd love for this functionality to land, in particular to communicate images and volumetric data more efficiently. In addition to being more memory efficient, it would also improve the speed of the JSON encoding, as we found during our work on improving the performance of the JSON encoder in #2880. I'm happy to get my hands dirty to help move this effort forwards. E.g. by implementing the encoding step for the Python JSON encoder. Though that should come after this. In the mean time, I could perhaps help implement support for other traces? |
I just did some benchmarking on the encoding of ndarray data in Python. Perhaps this helps create some enthusiasm for this feature :) I used a test array of 1000x1000 containing random data. I compared encoding the data in the form proposed in this PR against letting the PlotlyJSONEncoder converting it to lists. The timing includes the time of the base64 encoding.
(this is measured with the improved PlotlyJSONEncoder of #2880, before that, the speed difference was even a factor 25). |
Yeah, I'm interested in picking this up again. Probably early December. @almarklein Thanks for chiming in and for trying out those mini benchmarks! One not yet settled question, as far as I understand, is what multi-dimensional array buffers are going to be de-serialized into in JavaScript (since TypedArrays are 1D only). @archmoj, I believe we already have a dependnecy on |
Right now one could pass 2D/3D arrays to volume, surface, etc. using 'shape' as mentioned in the modified attribute description. |
@archmoj Could you explain your motivation here? If I remember correctly, it seemed easier to me to do this in the top-level Thanks again for resurrecting this! |
supplyDefault is called during interactions. So basically we don't want to run slow processes on arrays in there. |
One issue I'm running into with having decoding happen in calc. This does prevent the decoding from happening during interactions, but since supplyDefaults does run during interactions, the typed arrays get overridden by the buffer specification in You can see the effect of this in your demo. When you click to change the modebar tool the isosurface disapears. This is because Not sure the best way forward here. Is there some way to skip supplyDefaults on an attribute that isn't going to have calc run on it? |
Works for initial render, but typed array is overwritten when supplyDefaults is run again.
This reverts commit 5079fc7
Given the speed improvements, this would be great to get in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 Let's do it! Great work, this is a long time coming!
Wooot! nice job folks! :) |
Architecture Details
The approach is to perform the typed array decoding in coerce (before the supplyDefaults logic in the layout and traces). This has the advantage that none of the supply defaults or calc logic needs to be aware of the typed array specification objects.
For performance, the ArrayBuffer instances resulting from the decoding process are cached by trace uid and property name. To keep cache bounded, only one ArrayBuffer is cached per trace per property. Playing with the isosurface example in #5230, building the typed array with this caching approach is about 10x faster than when performing the base64 decoding.
Implementation Details
This PR handles decoding N-dimensional arrays into a nested collection of
Arrays
. The inner most layer contains TypedArrays that are backed by the original decoded ArrayBuffer, so no copying is performed. So far, all the 2D traces I've played with have worked fine with this nesting arrangement.More testing is needed, but hopefully nothing in
supplyDefaults
or calc or rendering will need to change in any of the traces (apart from potential bug fixes for TypedArray handling).Latest demo
codepen
Previous attempt
The initial commit dc9f4e5 was inspired by @jonmmease's PR #2911.
@plotly/plotly_js