perf(calcTransformMatrix): replace cache key string with array

[jiayihu]

The current cache key strategy is very expensive because it creates long string, especially with grouped objects as each parent adds a same length prefix. Long strings are expensive to concatenate and require Garbage Collection, which steals CPU time and makes rendering performance less consistent, as the GC runs at unpredictable times.

Threejs had a similar issue with cache keys mrdoob/three.js#22530, which however they fixed in mrdoob/three.js#22960, which I don't think are suitable for us.

My proposal instead is to replace the cache key with an array of the direct values as numbers, so that checking by cache key is just a check for numbers. There are probably better strategies, such as:

• update the transform matrix when rendering only
• update the transform matrix when set() is called with props affecting the transform
• rely on dirty flag to be accurate, especially when groups are updated, to cache the transform

All these alternatives require discussion and are too risky/effort since we're in release-candidate phase. This proposal instead has the only breaking change of changing the cache key to be an array. Nobody should have relied on it, but in case they can simply convert the array to string.

https://codesandbox.io/p/sandbox/fabric-bench-forked-4w858t shows around 25% performance improvement with this simple change.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[jiayihu]

FYI this is a good discussion and summary of different strategies they discussed in three.js: mrdoob/three.js#14138 (comment)
FWIW I believe that since fabric already uses the dirty flag, it should use it for transform matrixes as well. But strategy 2 "cache position/quaternion/scale" is basically this PR, proving it's a valid option.

[asturur]

i have no doubt this is better than what we have.
I know what we have is trash.
Hopefully when origin is gone caching is not really necessary. What we want to really avoid is multiple calculation of the same object per render, so maybe there are also easier solutions.

I ll have to do the same bench file so if you want to move it forward start to prepare it yourself, i ll be doing a couple of other things next 2-3 days

[asturur]

i would argue that angle could go right away top and left, then width, height, scaleX and scaleY and then all the rest. Origin last. Is probably non influent but i would have the properties that have a chance to change more often at the top of the array.

I also think this list could be flawed for the exactbounding box case

[jiayihu]

Done, I've sorted properties by likelihook of change. I put angle after scale as I think it's less probable. People usually move, resize, scale objects and then less frequently rotate them.

An advantage of this method is also that it's easy to override in the app and sort/remove the properties according to the app.

I also think this list could be flawed for the exactbounding box case

Not sure to understand what you're referring and what would be the action

[asturur]

i meant that when exactBoudning box is in play all the stroke join properties influence position and so the matrix.
That could be source of super hidden bugs that we think are calculation issues and maybe are not. But this is a pre-existing bug ad has nothing to do with this PR

[jiayihu]

FYI: my PRs are the result of the analysis described in https://blog.jiayihu.net/comprenhensive-guide-chrome-performance/ I'll slowly try to apply the changes in the article into fabric

[asturur]

please add a file in the benchmark folder like i did for the other PR. is quick to do i won't be able to do it because i have a lot of busy days

[jiayihu]

Done. Here are the results as well:

{ complexOld: 742, complexNew: 393 }
{ simpleOld: 191, simpleNew: 101 }

Basically a consistent 50% improvement.

[asturur]

oh great i was planning to fix the test today