Skip to content

Test speedups - #10042

Closed
akx wants to merge 6 commits into
python-pillow:mainfrom
akx:test-speedups
Closed

akx wants to merge 6 commits into
python-pillow:mainfrom
akx:test-speedups

Conversation

@akx

@akx akx commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

On my machine, this speeds up the entire test suite by ~8% on average, when running on all cores. (hyperfine)

~/b/Pillow (main) $ uv pip install -e . && hyperfine 'uv run -m pytest -nlogical --dist=worksteal'
Benchmark 1: uv run -m pytest -nlogical --dist=worksteal
  Time (mean ± σ):     11.237 s ±  0.323 s    [User: 61.079 s, System: 10.519 s]
  Range (min … max):   10.719 s … 11.919 s    10 runs

~/b/Pillow (test-speedups) $ uv pip install -e . && hyperfine 'uv run -m pytest -nlogical --dist=worksteal'
Benchmark 1: uv run -m pytest -nlogical --dist=worksteal
  Time (mean ± σ):     10.419 s ±  0.424 s    [User: 58.852 s, System: 10.216 s]
  Range (min … max):   10.048 s … 11.393 s    10 runs

@akx

akx commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

The reason I started looking at assert_image_similar was this tracy-via-tracypy fragment:

Screenshot 2026-09-22 at 18 02 14

@akx

This comment was marked as outdated.

Comment thread Tests/test_file_apng.py
default_image=True,
append_images=frames,
)
assert test_file.read_bytes().count(b"fdAT") > 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we have a difference of opinion on this, but I still don't see minor performance improvements in the test suite as a reason to start monkeypatching. I would rather test Pillow as it is with a real image.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pillow is not being monkeypatched. MAXBLOCK is a documented public API, this just sets it temporarily.

This is the same pattern as used in e.g. test_padded_idat in test_file_png.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...when I said 'monkeypatching', I was referring to monkeypatch.setattr(ImageFile, "MAXBLOCK", 1024)

#5493 did that in order to avoid a large test image being stored permanently in the repository.

This change is about avoiding handling a 4160x870 image in memory, which doesn't seem that extreme to me.

@akx akx Sep 28, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - here MAXBLOCK is being set to force writing smaller chunks to satisfy the premise of their test.

I didn't think about it also affecting reading, so if you'd like, we could undo the patch before reading them file back in? EDIT: Did that in a subsequent commit.

And, EDIT:

This change is about avoiding handling a 4160x870 image in memory, which doesn't seem that extreme to me.

No, it's not that extreme, but spending those cycles is also unnecessary for satisfying the premise of the test. Another option to speed up the test (while not doing anything about the memory cost) is to use the large image, and tell the encoder to not waste time compressing it.

@radarhere

radarhere commented Oct 23, 2026 •

Copy link
Copy Markdown
Member
  • Regarding the 'stray globals', I've cherry-picked that into Remove unnecessary code #10292
  • Regarding convert_to_comparable(), looking at that code, I think it's incorrect to compare two P images by discarding their palette data. I think it would be better to convert them to RGB first. See Convert P images to RGB before comparing #10290
  • For the assert_image_similar() speedup, I found that because histogram() only groups data into 256 bins, the computed difference of 255. For I mode images, the difference could be much larger than that. Obviously, that flaw already exists in main, and yes, our epsilon threshold should never be that high, but I would still not like to add code dedicated to I mode images in there, as it would give the impression that we considered this mode and deemed the output correct.
  • For test_apng_save_split_fdat(), as I've said in comments above, I don't think the test is extreme, and so would prefer simplicity and realism.
  • I'm tempted by the parametrization of test_parser - not for performance, but just to split up a large test - but don't really like having the formats listed in one place with their skip conditions and then having the specific conditions for those formats in another.

@radarhere radarhere closed this Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants