Speed up Image.getextrema() - #9762
Conversation
Merging this PR will improve performance by ×3
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_getextrema[1237x811-CMYK] |
20.3 ms | 4.7 ms | ×4.3 |
| ⚡ | test_getextrema[1237x811-RGBA] |
20.3 ms | 4.7 ms | ×4.3 |
| ⚡ | test_getextrema[1237x811-LA] |
15.7 ms | 4.7 ms | ×3.3 |
| ⚡ | test_getextrema[1237x811-RGB] |
14.2 ms | 4.7 ms | ×3 |
| ⚡ | test_getextrema[1237x811-L] |
3.6 ms | 1.4 ms | ×2.5 |
| ⚡ | test_getextrema[1237x811-I] |
6.7 ms | 4.3 ms | +55.06% |
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:extremaly-fast (6002bd5) with main (0d20a00)
Footnotes
-
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. ↩
| static PyObject * | ||
| _getextrema(ImagingObject *self, PyObject *args) { | ||
| if (self->image->type == IMAGING_TYPE_UINT8 && self->image->bands > 1 && | ||
| self->image->bands <= 4) { |
There was a problem hiding this comment.
Considering that we don't have any images with more than 4 bands, I'm not sold that we need to check for it.
There was a problem hiding this comment.
If this check is elided, in the future when someone decides to add such support, the following code will crash in unfortunate and non-obvious ways when it writes over the UINT8 mb[2 * 4]; array, where 4 stands for "maximum number of bands supported by this code".
I can remove the check if you like, but I don't think it costs much here.
There was a problem hiding this comment.
Couldn't you just allocate mb to match the number of bands?
There was a problem hiding this comment.
Using a VLA (or heap allocation) becomes a little complex, since due to the data layout of 2-band (PA/LA/La) images (pixelsize 4, XX....AA in image32), the actual valid sizes for mb (and ImagingGetExtremaMultiband's vmin and vmax) are actually only 3 or 4. IOW, the required accumulator array size is not directly related to bands.
I'm not sure it's worth the complexity, what do you think?
Speeds up
getextrema()-- with more than just hoist and restrict this time, by adding a separate function for the common case of 2-4 band 8bpc images. The single-channel case speeds up pleasantly too by autovectorisation, though.