Skip to content

gh-134635: add zlib.{adler32,crc32}_combine to combine checksums - #134650

Merged
picnixz merged 10 commits into
python:mainfrom
picnixz:feat/zlib/crc32-combine-134635
May 27, 2025
Merged

gh-134635: add zlib.{adler32,crc32}_combine to combine checksums#134650
picnixz merged 10 commits into
python:mainfrom
picnixz:feat/zlib/crc32-combine-134635

Conversation

@picnixz

@picnixzpicnixz commented May 25, 2025

Copy link
Copy Markdown
Member

Comment threadModules/zlibmodule.c

@emmatypingemmatyping left a comment

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.

Have a few suggestions/thoughts but overall looks great!

Comment threadDoc/library/zlib.rst Outdated
Comment threadDoc/library/zlib.rst Outdated
Comment threadDoc/library/zlib.rst Outdated
Comment threadDoc/library/zlib.rst Outdated
Comment threadDoc/whatsnew/3.15.rst Outdated
Comment threadDoc/library/zlib.rst
Comment threadLib/test/test_zlib.py
Comment threadModules/zlibmodule.c
Comment threadModules/zlibmodule.c Outdated
Comment threadModules/zlibmodule.c Outdated
@picnixz

Copy link
Copy Markdown
MemberAuthor

Thanks for the feedback! I'm not on my dev session so I won't be able to change the clinic-related code as I'll need to regen the files, but I'll address the rest in the meantime.

picnixzand others added 3 commits May 25, 2025 17:34
Comment threadModules/zlibmodule.c Outdated
Comment threadModules/zlibmodule.c Outdated

@thatchthatch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall looks good, thanks!

@picnixz
picnixzforce-pushed the feat/zlib/crc32-combine-134635 branch from aa8b941 to c5a2c55CompareMay 26, 2025 09:03
@picnixz

Copy link
Copy Markdown
MemberAuthor

Thanks for the reviews both of you!

@picnixz
picnixz requested review from emmatyping and thatchMay 26, 2025 09:25

@emmatypingemmatyping left a comment

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.

Two suggestions for wording but otherwise looks great! Thank you for adding these.

Comment threadDoc/library/zlib.rst Outdated
Comment threadDoc/library/zlib.rst Outdated
Co-authored-by: Emma Smith <emma@emmatyping.dev>

@picnixzpicnixz left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I've edited your suggestion a bit but thank you!

@picnixzpicnixz self-assigned this May 26, 2025
@picnixz
picnixz merged commit 737b4ba into python:mainMay 27, 2025
@picnixz
picnixz deleted the feat/zlib/crc32-combine-134635 branch May 27, 2025 08:48
@morotti

Copy link
Copy Markdown
Contributor

However, looking at the fastzip code, they are concurrently calculating the checksums and assembling them afterwards, so they cannot chain calls to zlib.crc32.

Just a note that the function is probably obsolete. The ticket goes back to 2019.
It's fine to add, though it might be misleading for suggesting that computing crc in parallel is a thing.

The python interpreter has switched to zlib-ng on windows (other OS to follow). the zlib.crc is computing crc with hardware instructions on x64, which are insanely fast. binascii.crc is just an alias to zlib.crc.
seeing up to 420GB per second on small data that fit in cpu cache on my desktop. (not a typo, it's insane!). nearly 100 GB per second otherwise.

there is little point in computing crc32 in parallel. In fact we should probably remove or adjust the crc code to not release the GIL, as it's counterproductive on small data.

@emmatyping

emmatyping commented Jun 26, 2025

Copy link
Copy Markdown
Member

@morotti

The python interpreter has switched to zlib-ng on windows (other OS to follow).

I don't think adopting zlib-ng everywhere is settled, and there is a long tail of old Linux distributions that want to link against a system zlib that do not ship zlib-ng yet. I don't think we can assume zlib-ng performance. That being said even zlib crc32 is pretty fast. However, even with a fast sequential crc32 I think this could be useful for concurrent checksuming for say, parallel chunked file downloads.

In fact we should probably remove or adjust the crc code to not release the GIL, as it's counterproductive on small data.

I would want to see convincing benchmarks before making this change and proper motivation, otherwise I'm not sure if this is worth the complexity. (if you have those though please do open an issue!)

@morotti

Copy link
Copy Markdown
Contributor

hello @emmatyping and sorry for pinging this old ticket

That being said even zlib crc32 is pretty fast.

Nope, zlib crc32 is horribly slow. I've been working on optimizing pip and whole percents of the duration of "pip install" is computing crc!

You can find benchmarks (both compression and crc) in the zlib-ng thread:
TL;DR around 20GB/s crc on slow laptop, 80GB/s crc on fast desktop, up to 420GB/s on very small data that can fit in cache.
#91349 (comment)
#91349 (comment)

I don't think adopting zlib-ng everywhere is settled, and there is a long tail of old Linux distributions that want to link against a system zlib that do not ship zlib-ng yet. I don't think we can assume zlib-ng performance.

See the thread, it's settled and there is agreement to do it.
The issue is nobody has figured out how to modify the build system on Linux/MacOS :D (and even if modified, some distributions will dynamic link the library and load zlib from the OS instead, as you say)

Anyway, we can get better crc function aside of zlib/zlib-ng. The python interpreter can use the crc32 function from liblzma instead.
This PR is exposing the function #131721

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
gostak-dd pushed a commit to gostak-dd/cpython that referenced this pull request Jun 2, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@picnixz@morotti@emmatyping@thatch