fix() Addressing path cloning slowness ( partially )

[asturur]

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:

{ pathCloneDeep: 650, pathOldCode: 563 }
{ cloningSimple: 770, cloningCustom: 25 }
{ cloningSimpleNormal: 141, cloningCustomNormal: 6 }

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

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	907.977 (+0.021)	305.164 (+0.016)

[asturur]

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 )

[ShaMan123]

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?

[asturur]

If i try it in the real test scenario of an app what i get is this, around 10% reduction ( on an average of 3 repetitions for each case ). This change also include the console not spamming warning, i have no idea how much that influences on the JS execution time
Current fabricJS

This branch

[ShaMan123]

I would add a test from cloning objects and then introduce lodash to see what is faster

[asturur]

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 don't to add dependencies and then worry for patches. I would like to stay dependency free for browser.
We have 2 cloneDeep cases remaining, one is points and should probably be handled like this, the other is styles and i have no idea how ugly it gets.

[ShaMan123]

I see
stylesToArray doesn't really need that and can be done in a better way.
Maybe the same applies to _fromObject
In setDragImage it can be dropped in favor of a spread IMO
And the polyline points as you mentioned

[asturur]

i added a case for lodash to rule out lazyness in exploring, but is slower anyway
i tested both the 4.5 single package and the version we have in our deps because of tests scripts.

{ pathCloneDeep: 641, pathOldCode: 492 }
{ cloningLodash: 366, cloningSimple: 781, cloningCustom: 24 }
{
  cloningLodashNormal: 54,
  cloningSimpleNormal: 142,
  cloningCustomNormal: 6
}

[github-actions]

Coverage after merging cloning-path-slowness into master will be

82.77%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.85%	94.83%	87.10%	97.14%	109, 112, 215–216, 241–242
CommonMethods.ts	96.55%	87.50%	100%	100%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	74.14%	65%	66.67%	82.76%	131, 139, 139, 139, 139, 139–141, 152–154, 30
constants.ts	100%	100%	100%	100%
src/LayoutManager
LayoutManager.ts	85.53%	89.83%	69.23%	87.84%	135–136, 138–139, 139, 139, 139, 139, 233, 296–297, 308, 47, 68, 86
constants.ts	100%	100%	100%	100%
src/LayoutManager/LayoutStrategies
ClipPathLayout.ts	82.05%	72.22%	100%	88.89%	30–31, 42, 54, 57–58, 65
FitContentLayout.ts	100%	100%	100%	100%
FixedLayout.ts	20%	0%	0%	40%	20–21, 23, 23, 23, 23, 23
LayoutStrategy.ts	86.54%	64.71%	100%	96.30%	36, 43, 43, 43, 51, 78–79
utils.ts	89.47%	80%	100%	100%	28, 34
src/Pattern
Pattern.ts	89.74%	91.89%	80%	90.32%	119, 130, 139, 32, 36, 95
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.06%	82.35%	100%	93.81%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79%	76.99%	83.05%	79.84%	1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	93.21%	91.60%	94.44%	94.22%	1013, 1021, 1140, 1142, 1144–1145, 302, 472–473, 475–476, 476, 476, 525–526, 587–588, 601, 641–643, 675, 680–681, 708–709, 769–770, 775–779, 781, 940, 940–941, 944, 964, 964
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1019, 1029, 1081–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
StaticCanvasOptions.ts	100%	100%	100%	100%
TextEditingManager.ts	86%	71.43%	91.67%	91.67%	17, 17, 17–18, 18, 18
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	97.50%	88.89%	100%	100%	33
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	94.44%	93.10%	91.67%	96.77%	183, 249, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	8.57%	0%	0%	15.79%	100, 106, 111, 126, 126, 126, 126, 126, 128–131, 131, 134, 141, 20, 28–32, 32, 32, 32, 32, 32, 32, 32, 53–59, 59, 59, 59, 59, 61, 66–67, 69, 79, 85–87, 87, 89, 92–93, 93, 93, 93, 93, 95
rotate.ts	19.57%	12.50%	50%

[ShaMan123]

what about the rest?

[jiayihu]

This change also include the console not spamming warning, i have no idea how much that influences on the JS execution time

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 structuredClone now natively: https://developer.mozilla.org/en-US/docs/Web/API/structuredClone

[ShaMan123]

Regarding deep cloning, there is structuredClone now natively: https://developer.mozilla.org/en-US/docs/Web/API/structuredClone

Very useful! We should use that

[asturur]

is out of range for now, requires polyfills.
We can benchmark to get an idea for the future

[asturur]

what about the rest?

Sorry didn't see this.
At the first occasion we can update.
This was a specific issue that was hitting me hard and was a quick fix. But i need to prioritize the release of 6.0 this is an improvement that can go out anytime. Anyone can open a PR for it. One per points, one per text style