Speed up some conversions

[akx]

Experiments to speed up the other conversions didn't really yield fruit.

[hugovk]

Rebased following benchmark image update: #9831.

Edit: or not, due to conflict :)

Merging main in instead.

[codspeed]

Merging this PR will improve performance by 76.98%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 4 improved benchmarks
✅ 599 untouched benchmarks
⏩ 335 skipped benchmarks¹

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	test_convert[1237x811-P-RGBA]	21.5 ms	10.4 ms	×2.1
⚡	test_convert[1237x811-P-RGB]	19.5 ms	10.7 ms	+82.85%
⚡	test_convert[1237x811-PA-RGB]	22 ms	13.2 ms	+66.8%
⚡	test_convert[1237x811-PA-RGBA]	23.9 ms	15.3 ms	+56.38%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing akx:faster-convert-x (b31fed7) with main (0a61c53)}

Footnotes

1. 335 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[radarhere]

Suggested change

	#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | (alpha))
	#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | alpha)

[akx]

Same here - defensive parentheses if alpha expands to an expression.

EDIT: Prior art in e.g. the B16/L16/S16 macros in _imaging.c.

[radarhere]

Suggested change

	#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | ((UINT32)(alpha) << 24))
	#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | ((UINT32)alpha << 24))

[akx]

Rather not, because alpha could be a more complex expression.

EDIT: Prior art in e.g. the B16/L16/S16 macros in _imaging.c.

[radarhere]

Could you mention here what your thinking was behind the new tests?

[akx]

Could you mention here what your thinking was behind the new tests?

• test_i_to_l_saturation: a naive implementation might just & 0xFF a more-than-8-bit value to make it fit, which would cause wrapping instead of saturation.
• test_p_to_rgb_exact: tests for the behavior of SET_ALPHA_32, which has that little-endian/big-endian trickery. Also codifies the current behavior of how a P mode image with a palette that has alpha gets converted (which is also touched upon in Ensure Image and core palettes are in sync after PA conversion #9834's new test)

[radarhere]

• test_i_to_l_saturation: a naive implementation might just & 0xFF a more-than-8-bit value to make it fit, which would cause wrapping instead of saturation.

You mean on main

diff --git a/src/libImaging/Convert.c b/src/libImaging/Convert.c
index 4a35a9a1f..bab3e7bc0 100644
--- a/src/libImaging/Convert.c
+++ b/src/libImaging/Convert.c
@@ -594,7 +594,7 @@ i2l(UINT8 *out, const UINT8 *in_, int xsize) {
         if (v <= 0) {
             *out = 0;
         } else if (v >= 255) {
-            *out = 255;
+            *out = v & 255;
         } else {
             *out = (UINT8)v;
         }

? That would cause two already existing tests in test_file_gif.py to fail.

• test_p_to_rgb_exact: tests for the behavior of SET_ALPHA_32, which has that little-endian/big-endian trickery.

Guessing at what you think we might get wrong on this branch,

diff --git a/src/libImaging/Convert.c b/src/libImaging/Convert.c
index 3d0653139..72e2a93d6 100644
--- a/src/libImaging/Convert.c
+++ b/src/libImaging/Convert.c
@@ -979,9 +979,9 @@ pa2f(UINT8 *out_, const UINT8 *in, int xsize, ImagingPalette palette) {
 
 // Set the alpha channel of the UINT32 `v` in-place to the given value.
 #ifdef WORDS_BIGENDIAN
-#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | (alpha))
+#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | (alpha))
 #else
-#define SET_ALPHA_32(v, alpha) v = ((v & 0x00FFFFFFu) | ((UINT32)(alpha) << 24))
+#define SET_ALPHA_32(v, alpha) v = ((v & 0xFFFFFF00u) | ((UINT32)(alpha) << 24))
 #endif
 
 static void

causes 36 tests to fail, including Tests/test_image_convert.py::test_rgba_pa.

I don't naturally expect a performance PR to add more tests, since no functionality should be changed.

You're adding tests not for coverage, basic correctness, or for regression checking, but to ensure that we don't make a hypothetical mistake in the future? I may be wrong, but that sounds like a step too far to me.

[akx]

[...]? That would cause two already existing tests in test_file_gif.py to fail.

Fair enough. Removed.

Guessing at what you think we might get wrong on this branch, [...] causes 36 tests to fail, including Tests/test_image_convert.py::test_rgba_pa.

Thanks for checking! Removed as well. The big-endian architecture Docker CI tests should then scream too. 👍

You're adding tests not for coverage, basic correctness, or for regression checking, but to ensure that we don't make a hypothetical mistake in the future?

Somewhat the other way around, really: to check that I didn't make mistakes within the branch. The new tests were stacked first in the branch's commits, so I could verify that they were green before optimizing things and after optimizing things.

[radarhere]

Does the fact that codspeed doesn't detect any significant difference for i2l() mean that those changes are unnecessary?

[akx]

@radarhere I wouldn't say so, as some of these depend on processor architecture:

On my laptop (macOS, M2 Max) there is a significant improvement there in addition to the ones also shown by CodSpeed (cropped out the rows that are < ±10%):

-------------------- benchmark 'convert': 26 tests, 2 sources -------------------
Name (time in us)                    0001_3bcb709 Min  0002_6dccfa0 Min      ΔMin
---------------------------------------------------------------------------------
test_convert[1237x811-I-L]                    86.2919           54.8750    -36.4%
test_convert[1237x811-PA-RGB]                820.7920          453.0830    -44.8%
test_convert[1237x811-P-RGB]                 832.7090          451.4590    -45.8%
test_convert[1237x811-PA-RGBA]               887.3750          472.0420    -46.8%
test_convert[1237x811-P-RGBA]                938.9170          384.2920    -59.1%
---------------------------------------------------------------------------------

This might be down to what compiler flags the benchmarked wheels end up being compiled with.

E.g. -mtune=native (if memory serves) would allow the compiler to use every fancy feature available on the (_{looks up CodSpeed} Intel(R) Xeon(R) 6973P-C the benchmark ran on, but then that wheel might not work on CPUs that don't have those features (they'd get SIGILL or similar). This was touched on in #8209.

Or it just might be that the compiler itself isn't smart enough to autovectorize that loop on that platform. And as we saw in #9654 (comment), CodSpeed doesn't like multiple runs of the same benchmark - otherwise it'd be very interesting to also run these benchmarks automatically on a macOS ARM runner!

[radarhere]

Hmm. I don't see a definite improvement on my macOS M4.

I'm used to #2217 being closed. Where exactly do you think the line is between premature optimization and a gain that doesn't appear on every OS when converting between two of our somewhat less popular modes?

[akx]

Hmm. I don't see a definite improvement on my macOS M4.

That's pretty curious. 🧐 Not sure what to attribute that to. Comparing the assembly of i2l on main and on this branch shows that my C compiler (Apple clang version 17.0.0 (clang-1700.3.19.1)) does heavier unrolling (a whole lot of smin.4s/smax.4s operations).

Can you do cc --version, I'm wondering where the difference is...

I found an interesting post about this though now that I started researching things: https://secret.club/2021/04/09/std-clamp.html

Where exactly do you think the line is between premature optimization and a gain that doesn't appear on every OS when converting between two of our somewhat less popular modes?

I'm not sure there's a definite line. For this case, the end result is exactly the same, it's less code (evidently an idiom some versions of clang recognize and do better with), and it doesn't show to be slower. If you like, I can of course revert it out of this PR?

[radarhere]

Oh, apologies, I think I was testing the wrong conversion.