-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(): Object dispose #8673
fix(): Object dispose #8673
Conversation
Build Stats
|
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.
Thoughts?
This is as much a PR as a discussion
Ready to merge if you don't disagree
} | ||
runningAnimations.cancelByTarget(this); | ||
this.off(); | ||
this._set('canvas', undefined); |
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.
using _set to propagate to all children
Came to me that disposing a canvas with a large tree with cost in perf since each object will be visited in the _set method |
I m not sure this PR is improving memory disposing, if we think it does we should be able to measure it in some way ( either from the heap allocation in the performance tab or in some other ways ). |
is ok if dispose do something and traverse a tree, disposing isn't used in render cycles and is something that is done once in a while. What it takes it takes. |
Motivation
#8671
Another scoped PR from #8665
Description
The whole point of the dispose method AFAIK is to release memory quickly
So removing the canvas ref and all the events (which probably have a lot of side effects) is important
Changes
Gist
In Action