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

Store parsed data more similarly to provided data #6766

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Nov 18, 2019

This has some of the changes from #6750, which we decided not to move forward with. I'm reopening for discussion purposes even if we decide to go in a different direction

I'd like to be able to skip all parsing if the user provides data in the correct format. Right now that format is essentially an array of objects. Other potential formats such as an array of arrays can be discussed in #6696. Right now, it seems most likely that we'll be accepting data as an array with an entry for each row.

This implements about half the change required to simply utilize user data as passed in. The other half would be to change the keys that the user data is being stored under. Right now we have keys like "x-axis-0" and would need to change that to "x" or change from an object to an array

This is probably about a wash in terms of performance. E.g. _getParsed is probably slightly faster and calculateTotal is probably slightly slower. Overall I think these effects are fairly negligible though

@kurkle
Copy link
Member

kurkle commented Nov 19, 2019

To me it seems that the internal data structure needs to be array of objects, to work in all use cases.

Performance wise I think its better to set _parsed in the Element directly to reference that original user provided object (instead of parsing and creating a new object), when some "data is prepared" option is set.

For x-scale-0 to x, I'd like to get #6626 done (specially the default of x / y scale id's)

Mapping by scale id leaves room for flexibility. If your data has something other than x and y for the values, you could rename the scales instead of mangling data. Not quite as flexible as input proposed in #6696 though.

But lets keep this open. We should measure performance both ways and use the better one!

Not that the array maintenance for _parsed is still missing from these places:

meta.data.splice(numData, numMeta - numData);

me._cachedMeta.data.splice(start, 0, ...elements);

this._cachedMeta.data.splice(start, count);

We should add tests using_getParsed, similar to this one (could be in the same test):

it('should synchronize metadata when data are inserted or removed', function() {

@benmccann
Copy link
Contributor Author

Putting _parsed on the element creates a reference per data point. I was thinking it'd be less memory and faster setup to just use the dataset directly. While I think there's potential for this to be more performant, I would expect that #6768 still captures most of the win

@benmccann benmccann closed this Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants