Skip to content
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(composeMatrix): 25% improv by restoring v5 implementation #9851

Merged
merged 7 commits into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Changelog

## [next]

- fix(util): restore old composeMatrix code for performances improvement [#9851](https://github.com/fabricjs/fabric.js/pull/9851)
- fix(Control): corner coords definition order [#9884](https://github.com/fabricjs/fabric.js/pull/9884)
- fix(Polyline): safeguard points arg from options [#9855](https://github.com/fabricjs/fabric.js/pull/9855)
- feat(IText): Adjust cursor blinking for better feedback [#9823](https://github.com/fabricjs/fabric.js/pull/9823)
Expand Down
104 changes: 104 additions & 0 deletions src/benchmarks/calcTransformMatrix.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { util } from '../../dist/index.mjs';

// perf(composeMatrix): 25% improv by restoring v5 implementation #9851

// OLD CODE FOR REFERENCE AND IMPLEMENTATION TEST

const util2 = { ...util };

util2.calcDimensionsMatrix = ({
scaleX = 1,
scaleY = 1,
flipX = false,
flipY = false,
skewX = 0,
skewY = 0,
}) => {
return util2.multiplyTransformMatrixArray(
[
util2.createScaleMatrix(
flipX ? -scaleX : scaleX,
flipY ? -scaleY : scaleY
),
skewX && util2.createSkewXMatrix(skewX),
skewY && util2.createSkewYMatrix(skewY),
],
true
);
};

util2.composeMatrix = ({
translateX = 0,
translateY = 0,
angle = 0,
...otherOptions
}) => {
return util2.multiplyTransformMatrixArray([
util2.createTranslateMatrix(translateX, translateY),
angle && util2.createRotateMatrix({ angle }),
util2.calcDimensionsMatrix(otherOptions),
]);
};

// END OF OLD CODE

const benchmark = (callback) => {
const start = Date.now();
callback();
return Date.now() - start;
};

const optionsComplex = {
skewY: 10,
skewX: 4,
scaleX: 5,
scaleY: 4,
angle: 20,
flipY: true,
};

const simpleCase = {
scaleX: 5,
scaleY: 4,
angle: 20,
};

const complexOld = benchmark(() => {
for (let i = 0; i < 1_000_000; i++) {
util2.composeMatrix(optionsComplex);
}
});

const complexNew = benchmark(() => {
for (let i = 0; i < 1_000_000; i++) {
util.composeMatrix(optionsComplex);
}
});

console.log({ complexOld, complexNew });

const simpleOld = benchmark(() => {
for (let i = 0; i < 1_000_000; i++) {
util2.composeMatrix(simpleCase);
}
});

const simpleNew = benchmark(() => {
for (let i = 0; i < 1_000_000; i++) {
util.composeMatrix(simpleCase);
}
});

console.log({ simpleOld, simpleNew });

/**
* On Node 18.17
* { complexOld: 749, complexNew: 627 }
* { simpleOld: 537, simpleNew: 374 }
*/

/**
* After removing the spread operator
* { complexOld: 761, complexNew: 446 }
* { simpleOld: 526, simpleNew: 271 }
*/
40 changes: 22 additions & 18 deletions src/util/misc/matrix.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,20 +277,24 @@ export const calcDimensionsMatrix = ({
skewX = 0 as TDegree,
skewY = 0 as TDegree,
}: TScaleMatrixArgs) => {
return multiplyTransformMatrixArray(
[
createScaleMatrix(flipX ? -scaleX : scaleX, flipY ? -scaleY : scaleY),
skewX && createSkewXMatrix(skewX),
skewY && createSkewYMatrix(skewY),
],
true
let matrix = createScaleMatrix(
flipX ? -scaleX : scaleX,
flipY ? -scaleY : scaleY
);
Comment on lines +280 to 283
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i m sure at some point we were checking for scale !== 1 and flip thruty to also do this.
I m not sure is worth to create it rather than returning the iMatrix as a reference and then check by strict equality.

calcOwnMatrix has been split for reusal across fabric but maybe was a bad idea, it was good as a code block doing its thing

Copy link
Contributor Author

@jiayihu jiayihu May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I m not sure is worth to create it rather than returning the iMatrix as a reference and then check by strict equality.

I thought about that but it's dangerous in fabric returning the same reference, as the iMatrix array could be mutated with terrible consequences

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iMatrix is frozen and will throw if you try to mutate it, we can't return it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but maybe it is quicker to create it than to clone it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't mutate matrix we always create new i don't see any issue with returning iMatrix when necessary. but anyway this PR is a revert basically, so is fine as it is it does not need more changes apart the code comments

if (skewX) {
matrix = multiplyTransformMatrices(matrix, createSkewXMatrix(skewX), true);
}
if (skewY) {
matrix = multiplyTransformMatrices(matrix, createSkewYMatrix(skewY), true);
}
return matrix;
};

/**
* Returns a transform matrix starting from an object of the same kind of
* the one returned from qrDecompose, useful also if you want to calculate some
* transformations from an object that is not enlived yet
* Before changing this function look at: src/benchmarks/calcTransformMatrix.mjs
* @param {Object} options
* @param {Number} [options.angle]
* @param {Number} [options.scaleX]
Expand All @@ -303,15 +307,15 @@ export const calcDimensionsMatrix = ({
* @param {Number} [options.translateY]
* @return {Number[]} transform matrix
*/
export const composeMatrix = ({
translateX = 0,
translateY = 0,
angle = 0 as TDegree,
...otherOptions
}: TComposeMatrixArgs): TMat2D => {
return multiplyTransformMatrixArray([
createTranslateMatrix(translateX, translateY),
angle && createRotateMatrix({ angle }),
calcDimensionsMatrix(otherOptions),
]);
export const composeMatrix = (options: TComposeMatrixArgs): TMat2D => {
const { translateX = 0, translateY = 0, angle = 0 as TDegree } = options;
let matrix = createTranslateMatrix(translateX, translateY);
if (angle) {
matrix = multiplyTransformMatrices(matrix, createRotateMatrix({ angle }));
}
const scaleMatrix = calcDimensionsMatrix(options);
if (!isIdentityMatrix(scaleMatrix)) {
matrix = multiplyTransformMatrices(matrix, scaleMatrix);
}
return matrix;
};
Loading