search

python / cpython

The Python programming language
https://www.python.org/
Other
60.87k stars 29.38k forks source link

Use zlib-ng or chromium's zlib rather than mainline stale zlib in binary releases #91349

Open gpshead opened 2 years ago

gpshead commented 2 years ago
BPO 47193
Nosy @gpshead, @pfmoore, @tjguk, @zware, @zooba, @corona10, @arhadthedev

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['3.11', 'OS-windows', 'performance'] title = 'Use zlib-ng rather than zlib in binary releases' updated_at = user = 'https://github.com/gpshead' ``` bugs.python.org fields: ```python activity = actor = 'corona10' assignee = 'none' closed = False closed_date = None closer = None components = ['Windows'] creation = creator = 'gregory.p.smith' dependencies = [] files = [] hgrepos = [] issue_num = 47193 keywords = [] message_count = 1.0 messages = ['416508'] nosy_count = 7.0 nosy_names = ['gregory.p.smith', 'paul.moore', 'tim.golden', 'zach.ware', 'steve.dower', 'corona10', 'arhadthedev'] pr_nums = [] priority = 'normal' resolution = None stage = 'needs patch' status = 'open' superseder = None type = 'performance' url = 'https://bugs.python.org/issue47193' versions = ['Python 3.11'] ```

gpshead commented 2 years ago

zlib-ng is an optimized zlib library with better performance on most architectures (with contributions from the likes of Google, Cloudflare, and Intel). It is API compatible with zlib. https://github.com/zlib-ng/zlib-ng

I believe the only platform we don't use the OS's own zlib on is Windows so I'm tagging this issue Windows.

gpshead commented 2 years ago

if this hasn't happened in the 3.11 betas this is presumably bumped to 3.12.

zooba commented 1 year ago

I started taking a look at this and it seems like we can build it without having to worry about their build system by renaming the zconf.h.in and zconf-ng.h.in files to remove the .in, and setting ZLIB_COMPAT preprocessor in pythoncore.vcxproj (as well as referencing the files).

Running test_zlib the only failures seem to be testing for certain failures that no longer fail, but I've got no idea how important they are. Also no indication of the performance impact, or anything else that may change (e.g. new DLL exports, etc.), but it certainly does seem like a feasible drop-in replacement.

rhpvorderman commented 1 year ago

@zooba which failures are these? I have accumulated quite some experience with the zlib/gzip formats due to working on python-isal, bindings for the ISA-L library that speeds up zlib-compatible compression by rewriting the algorithms in x86 Assembly language. It is quite good, but not suitable for a drop-in replacement, hence the python-isal project.

Having said that I'd love to help out with anything zlib related in CPython.

zooba commented 1 year ago

I knew I should've kept better track of the changed error messages 😄

Skimming through the tests, I'm pretty sure test_wbits failed in a few places because we expected some "invalid window size" errors that aren't errors with zlib-ng. I don't think any were critical, but I don't know the usage of the library well enough to be sure (e.g. do people expect these errors and change behaviour? or are they always just developer error and cause changes to just make things work?)

gpshead commented 1 year ago

Checking internally for local patches to zlib-ng, this change https://github.com/zlib-ng/zlib-ng/commit/ce6789c7e093e8e6bb6fc591bbdf0f805999bdb9 appears to fix that problem. I guess it hasn't landed in a zlib-ng release yet. (I don't see it in the 2.0.x branch)

Lets make sure that lands in zlib-ng before using it in our releases I guess.

Otherwise: I don't see any Python code using zlib.error that'd care (internally and in a large corpus of third party OSS python code). It is mostly only ever caught to bail out or follows this pattern to try the alternate meaning of wbits for zlib vs gzip format: https://github.com/urllib3/urllib3/blob/main/src/urllib3/response.py#L102

nmoinvaz commented 1 year ago

I started taking a look at this and it seems like we can build it without having to worry about their build system by renaming the zconf.h.in and zconf-ng.h.in files to remove the .in, and setting ZLIB_COMPAT preprocessor in pythoncore.vcxproj (as well as referencing the files).

I don't recommend bypassing the build system as it is used to determine features of the compiler and what optimizations to enable. If you only use Visual Studio then it is best to use cmake to generate your Visual Studio projects.

zooba commented 1 year ago

I don't recommend bypassing the build system

Yeah, I figured this would be a bad idea, but we're also not going to run a separate build system every time. We'll run it once and keep the generated files in our source mirror. (This extra work is why I didn't bother setting it all up earlier, and just hacked things enough to make it build.)

rhpvorderman commented 1 year ago

Is this change intended to remain Windows only? I am interested in packaging zlib_ng for python. (It will ship zlib_ng and gzip_ng modules and be fully compatible in every way, at least that is the plan). But this only makes sense if CPython is not going to switch to using zlib_ng by default any time soon. @gpshead if I understand you correctly, this change is merely for complete distributions that do not dynamically link other libraries at runtime?

gpshead commented 1 year ago

There's nothing stopping you from making zlib-ng wrapping modules on PyPI if you wanted to (check to see if anyone already has first). It'd allow applications on any Python version you provide wheels for to choose to use the faster library without rebuilding anything themselves.

Linux distros decide what zlib implementation they use for their own libz so that is not in our control, we don't want to vendor more third party libraries there if we can help it. On macOS I assume we're using a platform zlib as well? But it'd be more reasonable to ship our own different one there as we have to do that for some other libraries such as OpenSSL already.

But on Windows, we bring and build it all, so I suggest using zlib-ng on Windows only for starters.

zlib-ng is an API drop in compatible replacement for zlib. As is the zlib implementation you can find in the Chromium repository: https://chromium.googlesource.com/chromium/src/third_party/zlib/. Both are much faster than upstream stock zlib. (and better maintained)

We switched to chromium's zlib internally at work. It offers a "compatibility mode" that proces bit-identical compressed output to zlib at the cost of some performance. That made it a lot easier to move a huge codebase across tons of different systems over to as there were some things making the incorrect assumption that compressed output is canonical based on the input.

If Python runs into that kind of issue during beta releases, I'd suggest we step back and look at the chromium zlib in compatibility mode instead of zlib-ng.

Honestly people designing new systems are better off using zstandard for all lossless compression purposes. But the reason zlib exists and is used is compatibility with existing systems that only accept that format as their lowest common denominator compressed data format.

rhpvorderman commented 1 year ago

There's nothing stopping you from making zlib-ng wrapping modules on PyPI if you wanted to (check to see if anyone already has first).

I checked, no-one yet... At least not on PyPI. As the python-isal maintainer, I also feel I can do this with rather less effort than someone who is not as experienced with python's zlibmodule. So I am going to try.

zlib-ng is an API drop in compatible replacement for zlib. As is the zlib implementation you can find in the Chromium repository: https://chromium.googlesource.com/chromium/src/third_party/zlib/. Both are much faster than upstream stock zlib. (and better maintained)

We switched to chromium's zlib internally at work. It offers a "compatibility mode" that proces bit-identical compressed output to zlib at the cost of some performance. That made it a lot easier to move a huge codebase across tons of different systems over to as there were some things making the incorrect assumption that compressed output is canonical based on the input.

If Python runs into that kind of issue during beta releases, I'd suggest we step back and look at the chromium zlib in compatibility mode instead of zlib-ng.

That is an interesting thing to consider. Now I have to consider which one of those to wrap. Since zlib-ng is also available in conda (in the conda-forge channel) I think that is the most obvious choice for redistribution.

Honestly people designing new systems are better off using zstandard for all lossless compression purposes.

Depends. Intel's ISA-L igzip application compresses faster and at a better compression ratio than zstd can achieve at those speeds. It decompresses slower though. However one complete compress-decompress cycle is faster on igzip than zstd. So for my work (biofinormatics) where a lot of files are run trough multiple programs, this is most optimal. Also it is backwards-compatible with gzip. So it is quite a big win. For most other use cases though, I am inclined to agree that zstd is much better. Especially when that will get its own Assembly implementation (if that will happen).

rhpvorderman commented 1 year ago

Hi. The python-zlib-ng project is going well: https://github.com/pycompression/python-zlib-ng. First release should be soon.

A couple of observations I thought I'd share which have relevance to this issue:

Now these are not deal-brakers when you want a zlib alternative that can be used next to it to enhance performance, but I feel this is less desirable for CPython. A more regularly maintained, stable and predictable library is more appropriate.

If Python runs into that kind of issue during beta releases, I'd suggest we step back and look at the chromium zlib in compatibility mode instead of zlib-ng.

I think this is the way to go based on my experience so far. This does give people on windows the least surprise. Of course YMMV but I thought I would add my 2 cents and maybe save someone else some work in trying it out.

nmoinvaz commented 1 year ago

zlib-ng is not as drop-in as it name suggests: compression level 1 is vastly different, sacrificing compression ratio for speed. (Files can be 50% bigger than files compressed with zlib). In my opinion it is quite unacceptable for python's zlib module to have such a big change in behaviour between releases. A couple of percent is doable, but this is just not nice. Especially since the goal of compression is to have less data.

At level 1, our goal is to sacrifice compression ratio for speed and we do this using Intel's quick deflate algorithm. It is possible to disable this algorithm using -D WITH_NEW_STRATEGIES=OFF. This should also solve your 4th bullet point about the bug.

Has anybody done any compression/decompression performance benchmarking with Chromium's zlib? As far as I remember they weren't focused on improving compression, only decompression. Benchmark from the last release of zlib-ng can be found here, however it only compares against original mainline zlib.

rhpvorderman commented 1 year ago

At level 1, our goal is to sacrifice compression ratio for speed and we do this using Intel's quick deflate algorithm. It is possible to disable this algorithm using -D WITH_NEW_STRATEGIES=OFF. This should also solve your 4th bullet point about the bug.

Ah, that is equivalent to ISA-L's level 0. I see. Thank you for the hint. The problem at the 4th bullet point seems fixes on the latest develop branch though, so I gues I will just wait it out until there is a new release. I think those new strategies are a key feature of zlib-ng so I will leave them in for the python bindings.

It does sway my opinion for CPython though. If it can be made more "drop-in" by turning on some options it seems a good fit. Zlib-ng is so easy to build on windows. That can't be said for every C project out there.

As far as I remember they weren't focused on improving compression, only decompression.

That makes sense for a browser, right? It would be a bit weird if compression was a focus. I don't know if browsers regularly sent compressed packets? They do receive them quite a lot.

Dead2 commented 1 year ago

I thought I should chime in here, and address the point about spotty releases. That is largely my fault and has to do with some health issues I have been going through, combined with a lot of code refactors and a bad strategy for backporting changes to the stable branch that requires me to do a lot of manual work.

There is currently a pre-release for a new 2.0 release https://github.com/zlib-ng/zlib-ng/pull/1393, and it mostly lacks CI fixes so tests work again (The current plan for that is to copy over the new Github Actions files, remove CI runs that are incompatible with 2.0 and hopefully there will not be a lot more to fix manually). That 2.0 release will be the last 2.0 release that we backport changes to, after that there will only be important fixes if needed.

The plan is to release a 2.1 beta shortly after the next 2.0 release is out. Hopefully the zlib-ng core codebase is now stable enough (in terms of not needing more big refactors) that we can do a more rolling kind of releases for 2.1.x. If that pans out, we will have more regular releases, each with a smaller amount of changes and less chances for breakage between each release. That way there will also be little need for backporting.

I fully expect there to be bugs in the current 2.1 development branch due to the large amount of refactors. It has been working perfectly in several production systems for a long time now, but those do not exercise nearly all the various code paths that a full distribution adoption will see. The automated testing for 2.1 is quite extensive though, so I hope we did not miss anything truly important.

rhpvorderman commented 1 year ago

@dead2 I hope your health issues are resolved soon. Take care! Also thanks a lot for zlib-ng. It is quite an awesome library. I am especially awed with how easy it was to build and integrate for a library that contains various optimizations for various architectures.

Hopefully the zlib-ng core codebase is now stable enough (in terms of not needing more big refactors) that we can do a more rolling kind of releases for 2.1.x. If that pans out, we will have more regular releases, each with a smaller amount of changes and less chances for breakage between each release. That way there will also be little need for backporting.

If I may gave some unsollicitied advice: don't keep a stable branch with backports. It is an extra maintenance burden. The code is only ever going to be as good as its test suite. This means the branch with the most extensive testing is going to be the most suitable for production. Making regular releases from the development head while creating a new test for every bug that is reported is therefore a very sound strategy for stable releases where no regressions occur.

The automated testing for 2.1 is quite extensive though, so I hope we did not miss anything truly important.

I will run the python-zlib-ng test suite on the branch as well. That has 14000+ compatibility tests with zlib proper, so that also may catch it when something is amiss.

rhpvorderman commented 1 year ago

Issue with test_wbits should be fixed with the next stable release of zlib-ng.

Dead2 commented 1 year ago

Just wanted to let you know that we are about to release beta2 of zlib-ng 2.1 shortly (within a day or two unless a problem crops up). No significant bugs were found during beta1, but we did pull in a couple changes that warrant another beta.

Also, regarding level1 trading off more speed for lower compression, it can be disabled by setting the compiler define -DNO_QUICK_STRATEGY if you want the compression ratio to stay more in line with stock zlib. We do not have a configure/CMake option for disabling only deflate_quick, and none is planned.