Skip to content

WIP: Typed array encoding in supplyDefaults with Caching #5308

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

Merged
merged 9 commits into from
Nov 29, 2020

Conversation

jonmmease
Copy link
Contributor

Overview

This PR builds off of #5230, with a candidate solution to the problem outlined in #5230 (comment).

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

@jonmmease jonmmease marked this pull request as draft November 28, 2020 19:46

// Normalize shape to a list
if(Number.isInteger(v.shape)) {
coerced.shape = [v.shape];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we possibly support numeric strings here as well?

if(Number.isInteger(v.shape)) {
coerced.shape = [v.shape];
} else {
coerced.shape = v.shape;
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a good idea to drop this array support for shape attribute; while it cannot be a typedArray itself.
In that case for n dimensional arrays one could pass an string containing comma (or X) separated integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, I am using the shape to build a multi-dimensional array when the length is more than 1. Only the inner most layer of the multi-dimensional array is made up of typed arrays, but I pretty sure it'll be a lot better than nothing.

This reverts commit 5079fc7
The fact that elements aren't regular numbers, and aren't compatible with
the Math module functions would make this challenging.
@archmoj
Copy link
Contributor

archmoj commented Nov 29, 2020

Instead of targeting master, could you possibly open a PR to merge your work into #5230 branch. That would help us test and dev this.
Thank you!

@jonmmease jonmmease changed the base branch from master to handle-coded-typed-arrays November 29, 2020 20:01
@jonmmease
Copy link
Contributor Author

Base should be updated now. Thanks!

@archmoj
Copy link
Contributor

archmoj commented Nov 29, 2020

💃

@archmoj archmoj marked this pull request as ready for review November 29, 2020 20:10
@archmoj archmoj merged commit a0d980e into handle-coded-typed-arrays Nov 29, 2020
@archmoj archmoj deleted the jonmmease_typed_array_encoding branch November 29, 2020 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants