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 https://github.com/mrdoob/three.js/issues/22530, which however they fixed in https://github.com/mrdoob/three.js/pull/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[bot]]

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: https://github.com/mrdoob/three.js/pull/14138#issuecomment-459450627 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

[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