-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Conversation
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.
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 |
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.
Looks like createMetaDataset
and createMetaData
could be replaced with createMeta(type)
I like this too. Same with bubble chart radius which is missing a scale. |
Closed in favor of #6758 |
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 replacedThere seemed to be a bug in
insertElements
.addElementsAndReset
calledupdateElement
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 thatI'm not sure why doughnut was using
_val
instead of_parsed
. This changes that for consistency