GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||
GeezerVison is showing github.com — gh-134635: add `zlib.{adler32,crc32}_combine` to combine checksums by picnixz · Pull Request #134650 · python/cpython Conversation
|
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Emma Smith <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, thanks!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
aa8b941 to
c5a2c55
Compare
|
Thanks for the reviews both of you! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Emma Smith <[email protected]>
There was a problem 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.
There was an error while loading. Please reload this page.
Just a note that the function is probably obsolete. The ticket goes back to 2019. 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. 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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.
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!) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
hello @emmatyping and sorry for pinging this old ticket
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:
See the thread, it's settled and there is agreement to do it. Anyway, we can get better crc function aside of zlib/zlib-ng. The python interpreter can use the crc32 function from liblzma instead. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.