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.

Sign up for GitHub

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

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

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7891c0
perf(composeMatrix): restore v5 implementation
jiayihu May 5, 2024
5d65ab0
Use consistent naming
jiayihu May 5, 2024
d514fdb
Fix test error
jiayihu May 6, 2024
5280921
benchmark
asturur May 19, 2024
8eda3b4
a spread less
asturur May 19, 2024
a9d7d14
changelog
asturur May 19, 2024
65013aa
Merge branch 'master' into perf/compose-matrix
asturur May 19, 2024
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
31 changes: 19 additions & 12 deletions src/util/misc/matrix.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,14 +277,17 @@ 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 •
edited
Loading

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, createSkewXMatrix(skewY), true);
}
return matrix;
};

/**
Expand All @@ -309,9 +312,13 @@ export const composeMatrix = ({
angle = 0 as TDegree,
...otherOptions
}: TComposeMatrixArgs): TMat2D => {
return multiplyTransformMatrixArray([
createTranslateMatrix(translateX, translateY),
angle && createRotateMatrix({ angle }),
calcDimensionsMatrix(otherOptions),
]);
let matrix = createTranslateMatrix(translateX, translateY);
if (angle) {
matrix = multiplyTransformMatrices(matrix, createRotateMatrix({ angle }));
}
const scaleMatrix = calcDimensionsMatrix(otherOptions);
if (!isIdentityMatrix(scaleMatrix)) {
matrix = multiplyTransformMatrices(matrix, scaleMatrix);
}
return matrix;
};
Loading