Skip to content

Scale cleanup#6916

Merged
etimberg merged 1 commit intochartjs:masterfrom
benmccann:scale-const-let
Jan 5, 2020
Merged

Scale cleanup#6916
etimberg merged 1 commit intochartjs:masterfrom
benmccann:scale-const-let

Conversation

@benmccann
Copy link
Copy Markdown
Contributor

Made one variable private and removed another. Changed everything to use const and let

item = items[i];
width = item.width;
color = item.color;
const item = items[i];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The V8 (or even babel) probably fixes this, but essentially we are instructing to allocate new variable for each iteration (that will be garbage collected later). So i think it would be good practice to allocate outside loop and reuse.
Correct me if I'm wrong :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't think we'd be able to notice any impact here. Maybe if it were something we were doing for every data point, but there won't be many grid lines

Copy link
Copy Markdown
Member

@kurkle kurkle Jan 5, 2020

Choose a reason for hiding this comment

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

I agree that this is not a critical part. The amount of garbage generated is quite large and we should try to minimize that. Lets just keep that in mind :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, could be worth testing for some of the loops that go over all data points

@etimberg etimberg merged commit 224fc11 into chartjs:master Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants