-
-
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
perf(): Rework constructors to avoid the extra perf cost of current setup #9891
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Build Stats
|
…s into constructor-performace
I could offset the added code but i can't really get rid of getDefaults() because that function is still necessary for the 'includeDefaults' functionality during export. |
* Constructor | ||
* @param {String} text Text string | ||
* @param {Object} [options] Options object | ||
*/ | ||
constructor(text: string, options?: Props) { | ||
super(text, options); | ||
super(text, { ...IText.ownDefaults, ...options } as Props); |
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.
Can't avoid rebuilding the object for Textbox -> IText -> Text because of how text measuring works. All the defaults needs to be assigned before Text base class execute the measurement of the text.
There are 2 failing unit tests, hopefully are just a small undersight and not a fundamental issue |
with this branch i get With master: This happens in a tight loop with no pauses so not really a real use case but still numbers are definitely better especially for simpler classes. |
If i try to replicate the same on fabric 5.x in jsfiddle, this is what i get: for 100.000 Textbox around 0.75 seconds So we are still miles away from the original performance @Balearica i would appreciate if you could run this branch in your benchmark to see a real use case |
Just to throw an idea, what if Otherwise honestly I think it's okay for people to manually do |
The team chose to keep value per class. I wasn't excited but that is it, i m not rolling back that now. |
This PR a a side effect makes the workaround easier |
Moving also away from all the defaults per instance goes back to v5 level, with a difference of 1-2 ms, so definitely ok. |
Hopefully i didn't break anything shady. |
@asturur Is it intended that canvas classes don't do |
Another issue I'm facing is that there is no way to override |
Yes i noticed the InteractiveFabricObject issue too when i was working on this, and i thought i should export it. |
So in Object.ts we define a class named FabricObject that we export as BaseFabricObject and it has its own defaults ( that you can override ). From the file FabricObject we define FabricObject again, extending InteractiveFabricObject for the necessity of adding a mixin, but without a constructor. This new class has no defaults, so the static 'ownDefaults' i think it points to the InteractiveObject one, and you could in theory mutate it, no? InteractiveObject class as is is not export and should be exported. Intera |
No, static properties are defined on the exact class I believe. For sure I tried this already and it didn't work, but I could be wrong as always. EDIT: it seems like so: class B {}
class A extends B {}
A.staticProp = 2
B.staticProp // undefined
A.staticProp // 2 |
Anyway InteractiveObject should be part of the exports. That is certain |
What about this issue? |
Sorry i forgot to answer. |
Wait, i went to check the code, it seems it behaves like objects before the refactor. super();
Object.assign(
this,
(this.constructor as typeof StaticCanvas).getDefaults()
); and getDefaults() is the recursive one. Why doesn't look like used to you? |
You're right, I didn't see it 👍 |
Description
close #9852
There were (at least) 2 problems with the v6 object initialization.
getDefaults was recursive running up from leaf to root class, spreading the result each time.
Now Textbox calls Itext, then FabricText, then InteractiveObject, then FabricObject getDefaults.
The Object getDefaults() was the largest object of the all, spread once in its own function but then spread again every time it returned from InteractiveObject, FabricText, IText, TextBox so in the worst case there were 5 unnecessary spread operations.
the function getDefaults() has to stay because is necessary for exporting object and removing default values.
What changes now is that every constructor has the duty to assign its own options and defaults, to avoid assigning options more than once we stop passing options to the super() call.
Text / IText / Textbox are its own particular case because they need to assign all the defaults and options before doing the measurements. So in this case we are still spreading options together with default values, this could make worse restoring an object compared as before, but i don't really have a solution for this. In thise case the defaults value are small objects so hopefully all in all is still overall better performance compared to before.
The best option performance wise would be adding a third parameter to the constructor to carry defaults separated from options, but until that is necessary i prefer to not do it
In Action