[PATCH 0/1] MR2839: gdiplus: Improve performance of GdipInvertMatrix.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 Performance tests results: * Without optimization : 0.320s * With current optimizations: 0.245s Application used for testing: [gdiplusdisplay.exe](/uploads/995f3f5d6fbe24ee7ad2513bae51a04f/gdiplusdisplay.exe) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2839
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 --- dlls/gdiplus/matrix.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/dlls/gdiplus/matrix.c b/dlls/gdiplus/matrix.c index d69672bddcf..c5d5d70c648 100644 --- a/dlls/gdiplus/matrix.c +++ b/dlls/gdiplus/matrix.c @@ -182,38 +182,41 @@ GpStatus WINGDIPAPI GdipInvertMatrix(GpMatrix *matrix) { GpMatrix copy; REAL det; - BOOL invertible; TRACE("(%p)\n", matrix); if(!matrix) return InvalidParameter; - GdipIsMatrixInvertible(matrix, &invertible); - if(!invertible) - return InvalidParameter; - /* optimize inverting simple scaling and translation matrices */ if(matrix->matrix[1] == 0 && matrix->matrix[2] == 0) { - matrix->matrix[4] = -matrix->matrix[4] / matrix->matrix[0]; - matrix->matrix[5] = -matrix->matrix[5] / matrix->matrix[3]; - matrix->matrix[0] = 1 / matrix->matrix[0]; - matrix->matrix[3] = 1 / matrix->matrix[3]; - - return Ok; + if (matrix->matrix[0] != 0 && matrix->matrix[3] != 0) + { + matrix->matrix[4] = -matrix->matrix[4] / matrix->matrix[0]; + matrix->matrix[5] = -matrix->matrix[5] / matrix->matrix[3]; + matrix->matrix[0] = 1 / matrix->matrix[0]; + matrix->matrix[3] = 1 / matrix->matrix[3]; + + return Ok; + } + else + return InvalidParameter; } - det = matrix_det(matrix); + if (!(fabs(det) >= 1e-5)) + return InvalidParameter; + + det = 1 / det; copy = *matrix; /* store result */ - matrix->matrix[0] = copy.matrix[3] / det; - matrix->matrix[1] = -copy.matrix[1] / det; - matrix->matrix[2] = -copy.matrix[2] / det; - matrix->matrix[3] = copy.matrix[0] / det; - matrix->matrix[4] = (copy.matrix[2]*copy.matrix[5]-copy.matrix[3]*copy.matrix[4]) / det; - matrix->matrix[5] = -(copy.matrix[0]*copy.matrix[5]-copy.matrix[1]*copy.matrix[4]) / det; + matrix->matrix[0] = copy.matrix[3] * det; + matrix->matrix[1] = -copy.matrix[1] * det; + matrix->matrix[2] = -copy.matrix[2] * det; + matrix->matrix[3] = copy.matrix[0] * det; + matrix->matrix[4] = (copy.matrix[2]*copy.matrix[5]-copy.matrix[3]*copy.matrix[4]) * det; + matrix->matrix[5] = -(copy.matrix[0]*copy.matrix[5]-copy.matrix[1]*copy.matrix[4]) * det; return Ok; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2839
Instead of duplicating checks, it's better to move functional part of GdipIsMatrixInvertible() to some inlined helper. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_32968
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/matrix.c:
- - return Ok; + if (matrix->matrix[0] != 0 && matrix->matrix[3] != 0) + { + matrix->matrix[4] = -matrix->matrix[4] / matrix->matrix[0]; + matrix->matrix[5] = -matrix->matrix[5] / matrix->matrix[3]; + matrix->matrix[0] = 1 / matrix->matrix[0]; + matrix->matrix[3] = 1 / matrix->matrix[3]; + + return Ok; + } + else + return InvalidParameter; } - det = matrix_det(matrix); We are calculating `matrix_det` only once (previously it was done also inside GdipIsMatrixInvertible)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_32969
On Wed May 17 09:07:24 2023 +0000, Nikolay Sivov wrote:
Instead of duplicating checks, it's better to move functional part of GdipIsMatrixInvertible() to some inlined helper. I have tried such approach, but unfortunately I don't know how to do not calculate `matrix_det(matrix)` twice.
Maybe we could introduce function and provide calculated `det`, like: ``` static inline GpStatus check_det(GDIPCONST REAL det) { if (!(fabs(det) >= 1e-5)) return InvalidParameter; return Ok; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_32977
On Wed May 17 09:15:05 2023 +0000, Bartosz Kosiorek wrote:
I have tried such approach, but unfortunately I don't know how to do not calculate `matrix_det(matrix)` twice. Maybe we could introduce function and provide calculated `det`, like: ``` static inline BOOL check_det(GDIPCONST REAL det) { return (fabs(det) >= 1e-5); } ``` for ``` if(matrix->matrix[1] == 0 && matrix->matrix[2] == 0) ``` check we are already using that check for optimization. So with proposed optimization, we have limited that check to one. I don't see a way to do this optimization while leaving the logic in GdipIsMatrixInvertible intact. There isn't much logic in there to begin with, so to me it seems fine to duplicate.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2839#note_33018
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2839
participants (5)
-
Bartosz (@gang65) -
Bartosz Kosiorek -
Bartosz Kosiorek (@gang65) -
Esme Povirk (@madewokherd) -
Nikolay Sivov (@nsivov)