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

Performance optimizations when animations are disabled #6710

Merged
merged 2 commits into from
Nov 10, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Nov 7, 2019

Another ~100ms off from uPlot benchmark.
@benmccann consider this kind of optimization while working with elements

@kurkle kurkle changed the title Don't copy _model when animations are disabled Performance optimizations when animations are disabled Nov 8, 2019
@kurkle kurkle marked this pull request as ready for review November 8, 2019 10:24
etimberg
etimberg previously approved these changes Nov 8, 2019
var me = this;
if (animationsDisabled) {
me._view = me._model;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this works, but would probably better belong in transition. My understanding is that the main point of pivot is to reset _start and that the point of transition is to set _view (we should probably document the functions)

pivot and transition both have a big of junk in them right now that are mainly cause by Tooltip not properly initializing these variables in the way that other elements do. I looked a little bit at trying to untangle some of it earlier, but didn't have the time. I don't think pivot should be initializing _view (why does it only do it once? what about subsequent updates) and I don't think transition should have to initialize _view and _start (_view should probably be initialized in a constructor and _start is already set in pivot, so we should make sure Tooltip calls pivot like everything else does)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm planning to take a deeper look at improving the animation system, in the near future (when I have enough time). But it will be another PR, if I ever get it done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I wouldn't try to make any of the other changes in this PR. But maybe we should at least pass the animationsDisabled flag to transition in this PR so that we don't drift further from the original intentions of the methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so how about breaking it a little then? :)
pivot should work the same, animations disabled or not.
transition is not called when animations are disabled (for dataset elements, for now)

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

nice. the code is a lot cleaner now!

i'm not sure I understood what you meant about breaking animations

test/specs/core.element.tests.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Nov 9, 2019

"Break the animations" commit really broke those (and was not easily fixed). So rebased and dropped that commit.

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

thanks for this. this will be a big performance win

@etimberg etimberg merged commit 81f5cf4 into chartjs:master Nov 10, 2019
@etimberg etimberg added this to the Version 3.0 milestone Nov 10, 2019
@kurkle kurkle deleted the no-animation branch November 13, 2019 06:09
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