GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-134635: add `zlib.{adler32,crc32}_combine` to combine checksums by picnixz · Pull Request #134650 · python/cpython

Conversation

@picnixz

@picnixz picnixz commented May 25, 2025 •
edited by github-actions Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Export zlib.{adler,crc}32_combine #134635

📚 Documentation preview 📚: https://cpython-previews--134650.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
picnixz added 2 commits May 25, 2025 14:01
@picnixz
expose zlib.{adler32,crc32}_combine
f71608e
@picnixz
update docs
c8a96c8
@bedevere-app bedevere-app Bot added the awaiting core review label May 25, 2025
@bedevere-app bedevere-app Bot mentioned this pull request May 25, 2025
Export zlib.{adler,crc}32_combine #134635
Closed
@picnixz
picnixz requested a review from emmatyping May 25, 2025 12:38
picnixz
picnixz commented May 25, 2025
View reviewed changes
Comment thread Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

emmatyping
emmatyping reviewed May 25, 2025
View reviewed changes

@emmatyping emmatyping left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/whatsnew/3.15.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_zlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@picnixz

picnixz commented May 25, 2025

Copy link
Copy Markdown
Member Author

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.

emmatyping reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz and others added 3 commits May 25, 2025 17:34
@picnixz
Address doc review's feedback
c67d7ba
@picnixz @emmatyping
Update What's New
0540501
Co-authored-by: Emma Smith <[email protected]>
@picnixz
add a failing test
c5a2c55
thatch
thatch reviewed May 26, 2025
View reviewed changes
Comment thread Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

thatch
thatch reviewed May 26, 2025
View reviewed changes
Comment thread Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

thatch
thatch approved these changes May 26, 2025
View reviewed changes

@thatch thatch left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Overall looks good, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@picnixz
picnixz force-pushed the feat/zlib/crc32-combine-134635 branch from aa8b941 to c5a2c55 Compare May 26, 2025 09:03
picnixz added 3 commits May 26, 2025 11:20
@picnixz
update clinic
cc160a7
@picnixz
remove rST syntax in clinic
f3fca34
@picnixz
add type checks
2c34e5f
@picnixz

picnixz commented May 26, 2025

Copy link
Copy Markdown
Member Author

Thanks for the reviews both of you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@picnixz
picnixz requested review from emmatyping and thatch May 26, 2025 09:25
@picnixz
Merge branch 'main' into feat/zlib/crc32-combine-134635
86746bd
emmatyping
emmatyping approved these changes May 26, 2025
View reviewed changes

@emmatyping emmatyping left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/zlib.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@picnixz @emmatyping
Apply suggestions from code review
2753b18
Co-authored-by: Emma Smith <[email protected]>
picnixz
picnixz commented May 26, 2025
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

emmatyping reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@picnixz picnixz self-assigned this May 26, 2025
thatch
thatch approved these changes May 26, 2025
View reviewed changes
@picnixz
picnixz merged commit 737b4ba into python:main May 27, 2025
@bedevere-app bedevere-app Bot removed the awaiting core review label May 27, 2025
@picnixz
picnixz deleted the feat/zlib/crc32-combine-134635 branch May 27, 2025 08:48
@morotti

morotti commented Jun 25, 2025

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@emmatyping

emmatyping commented Jun 26, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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!)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@morotti

morotti commented Jun 26, 2025

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@picnixz @Pranjal095
pythongh-134635: add zlib.{adler32,crc32}_combine to combine checks…
67904d6
…ums (python#134650)
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@picnixz @taegyunkim
pythongh-134635: add zlib.{adler32,crc32}_combine to combine checks…
4b8b054
…ums (python#134650)
@emmatyping emmatyping mentioned this pull request Aug 30, 2025
bpo-38687: Expose zlib 'adler32_combine' function to Python zlib module #17048
Closed
gostak-dd pushed a commit to gostak-dd/cpython that referenced this pull request Jun 2, 2026
@picnixz @gostak-dd
pythongh-134635: add zlib.{adler32,crc32}_combine to combine checks…
68324f6
…ums (python#134650)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emmatyping emmatyping emmatyping approved these changes
+1 more reviewer
@thatch thatch thatch approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@picnixz picnixz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@picnixz @morotti @emmatyping @thatch
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.