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

Reduce object creation in parsing #6750

Closed
wants to merge 1 commit into from
Closed

Conversation

benmccann
Copy link
Contributor

There was a line (_parsed: {}) that created a new object for each index, which I think caused extra garbage collection since that object was never used and ended up getting replaced

There seemed to be a bug in insertElements. addElementsAndReset called updateElement which depended on parsing already having happened. It seems like it was able to complete before because it had the empty object to reference, but it was uncovered when I got rid of that

I'm not sure why doughnut was using _val instead of _parsed. This changes that for consistency

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

You'd need to add the array maintenance done to meta.data when inserting / deleting / resyncing elements to this new array.

I used _val in doughnut to make it clear that it's not something done by DatasetController._parse() and does not work the same way (no axis.id). But that change is fine by me.
I'd actually like to change the doughnut to use an scale to be consistent (I think some issues could be solved that way too, but am too lazy to look for them now)

What if we left it under meta.data, but instead of replacing it, just set the values in it? Wouldn't it be more efficient that way (removing the extra array maintenance). If going that way, then those meta creation functions cant be replaced.

We could maybe reduce the memory footprint by placing the parsed data directly to meta properties, instead of under object. Would need to prefix the variables somehow, to try avoiding collisions (like meta['_parsed'+axis.id]). Not sure if that would be more efficient compared to meta._parsed[axis.id]

@@ -339,8 +339,7 @@ helpers.extend(DatasetController.prototype, {
var me = this;
var type = me.dataElementType;
return type && new type({
_ctx: me.chart.ctx,
_parsed: {}
_ctx: me.chart.ctx
Copy link
Member

Choose a reason for hiding this comment

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

Looks like createMetaDataset and createMetaData could be replaced with createMeta(type)

@etimberg
Copy link
Member

I'd actually like to change the doughnut to use an scale to be consistent (I think some issues could be solved that way too, but am too lazy to look for them now)

I like this too. Same with bubble chart radius which is missing a scale.

@benmccann
Copy link
Contributor Author

Closed in favor of #6758

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.

3 participants