GNOME Bugzilla – Bug 585220
performance of gtk+ with medialib support should be improved
Last modified: 2018-05-19 22:03:11 UTC
I built 2 copy of gtk+ on Solaris, one with medialib support and another one without medialib support. I also modified timescale.c a little (change ITERS from 10 to 100000) to test the performance. According to the test result, the performance of gtk+ with medialib support enabled is worse than gtk+ without medialib support in 2 cases: 1) if filter_level is PIXOPS_INTERP_NEAREST 2) if src_channels is 4 and dest_channels is 3 and src_channels has no alpha.
Created attachment 136187 [details] test result without medialib support
Created attachment 136188 [details] test result with medialib support enabled
Created attachment 136193 [details] [review] patch to fix the problem if filter_level is PIXOPS_INTERP_NEAREST, calls pixops_composite_nearest or pixops_scale_nearest. if src_channels is 4 and dest_channels is 3 and src_channels has no alpha, , calls _pixops_scale_real. Removes the following block, because (!src_has_alpha && overall_alpha == 255) has been handled before entering pixops_medialib_composite. if (!src_has_alpha && overall_alpha == 255 && dest_channels <= src_channels) { pixops_medialib_scale (dest_buf, dest_region_width, dest_region_height, dest_rowstride, dest_channels, dest_has_alpha, src_buf, src_width, src_height, src_rowstride, src_channels, src_has_alpha, dest_x, dest_y, dest_region_width, dest_region_height, offset_x, offset_y, scale_x, scale_y, interp_type); return; } Also removes the old code to handle the cases of "filter_level is PIXOPS_INTERP_NEAREST" or "src_channels is 4 and dest_channels is 3 and src_channels has no alpha".
Created attachment 136194 [details] test result with new patch
Created attachment 136197 [details] [review] patch to fix the problem Sorry, I attached the wrong patch. This is the correct one.
Wang, you don't specify whether your performance reports were using Solaris x86 or Solaris Sparc. Could you specify? Also, it would be good to also provide some performance report for both x86 and Sparc. How does the performance compare between x86 and Sparc?
Also, in pixops.c, in the function _pixops_use_medialib, there is this comment: * The first 2 digits of the version are the major version. The 3rd digit * is the minor version, and the 4th digit is the micro version. So the * above string corresponds to version 2.1.0. In the following test we only * care about the major version. Now, in the code we don't actually check the version number. Now we just check the ISALIST identifier. It would be good to fix this comment to remove the statement "In the following test we only care about the major version". The file gdk/gdkmedialib.c has a similar comment which should also be fixed. If we are cleaning up the code, then it would be good to fix these comments. Also, the configure script checks for mediaLib 2.3 and 2.5. At this point, it is probably okay to remove mediaLib 2.3 support. Would be nice to also clean up the code to just support mediaLib 2.5 only.
Brian, the result is for x86. I will paste the result for sparc later.
Created attachment 136611 [details] test result with medialib support before applying my patch on sparc.
Created attachment 136612 [details] test result with medialib support after applying my patch on sparc
Thanks. Based on these results, the code looks good to me. Note that I was the author of the original mediaLib GTK+ integration code. Note that my issues in comment #7 are really not related to this change. If Wang provides an updated patch that resolves those issues, it would be nice, but should not hold up the integration of the patch. The other issues could be treated as a separate bug.
Can someone take a look at this bug?
In comment #7 I suggested a few more cleanups that should be made, but an updated patch was never provided.
I will update it soon.
Created attachment 142855 [details] [review] New patch
Thanks. This patch looks good, and, in my opinion, is ready to go upstream. I have verified that this does improve performance.
*** Bug 583767 has been marked as a duplicate of this bug. ***
Is this still useful?
Review of attachment 142855 [details] [review]: The patch should be git-formatted, with a commit message.
Closing temporarily for migration, as it makes the migration tool fail.