-
-
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() Addressing path cloning slowness ( partially ) #9573
Conversation
Build Stats
|
Added note: the result of this change in my particular case of the app may be exacerbated or the improvements can be less because of the frequent GC call i was getting during the clone process. if JSON.stringify was trashing more memory i will have less GC calls and so more improvements or the same number of GC calls and so less improvements. ( i ll try to give it a stab ) |
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.
So doesn't this mean we should introduce a better cloner in general?
I wanted to use lodash.cloneDeep back when we touched it.
Are you against? And if so why?
I would add a test from cloning objects and then introduce lodash to see what is faster |
I don't to add dependencies and then worry for patches. I would like to stay dependency free for browser. |
I see |
i added a case for lodash to rule out lazyness in exploring, but is slower anyway
|
… into cloning-path-slowness
what about the rest? |
That can affect the performance significantly depending on the browser because writing on the console is an I/O operation basically. So it depends if the browser accounts for that time and removes it. It's better to run benchmarks without console logs. In Node that's even worse of course, outputing to the console is definitely expensive I/O. Regarding deep cloning, there is |
Very useful! We should use that |
Sorry didn't see this. |
Description
While working on a fun pet project with fabricJS i noticed that cloning many groups slowed down the app very much.
The flameGraph showed clearly i was running many times one cloneDeep, so i decided to look into it.
The path i added in this benchmark is the large i have and i m not using yet in the project, while the normal one is a real one.
It turns out that the JSON.stringify/parse approach for cloning is very unefficient for things you can easily customize for specific loop.
Those are the results i got:
I can read as follow:
Creating and cloning a single large path is extremely slow in any case.
This process include 2 time parsing the path, 2 bounding box calculation, and the data structure clone.
But the only change in serialization of the large path account for around 80+ ms, that seems a LOT.
If i repeat the test for the cloning part only, on 100 iterations the path data cloning seems 30 times faster anyway,
If i take a simpler path, the difference is in the 20 times region.
So i m proposing to just bring back the small old code.
I know this is a synthetic benchmark, but when working with the app i did got slow, and the flamegraph pointed me exactly here, so is not that much of a stretch, is tangible.
In Action