Skip to content

Conversation

@ericstj
Copy link
Member

@ericstj ericstj commented Aug 6, 2025

Update: 8/7/2025 -- this is now ready to merge as zlib-ng published the release https://github.com/zlib-ng/zlib-ng/releases/tag/2.2.5

This is a trial of zlib-ng/zlib-ng#1943 with the goal of getting test coverage. We should update with final hash/tag once it's merged.

We should see if we can get performance results on this.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 6, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the zlib-ng library from version 2.2.4 to 2.2.5, marking this as a trial of an upstream pull request with the goal of getting test coverage. The update includes version string updates, build system improvements, and performance optimizations particularly for x86 and RISC-V architectures.

Key changes include:

  • Version number updates across multiple files and build configurations
  • Improvements to x86 feature detection and AVX512VNNI intrinsics handling
  • Enhanced RISC-V architecture support with new toolchain configurations
  • Build system refinements including additional compiler flag checks for Apple platforms

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/native/external/zlib-ng/zutil.c Updates version string from 2.2.4 to 2.2.5
src/native/external/zlib-ng/zlib.pc.in Adds bindir variable to pkg-config template
src/native/external/zlib-ng/zlib.pc.cmakein Adds bindir variable to CMake pkg-config template
src/native/external/zlib-ng/zlib.h.in Updates version constants and macros to 2.2.5
src/native/external/zlib-ng/zlib-ng.h.in Updates version constants and macros to 2.2.5
src/native/external/zlib-ng/deflate_medium.c Optimizes match insertion logic and hash table management
src/native/external/zlib-ng/deflate.h Updates comment about max_insert_length usage
src/native/external/zlib-ng/configure Adds RISC-V support and additional compiler flag checks
src/native/external/zlib-ng/cmake/toolchain-riscv.cmake Simplifies RISC-V toolchain configuration
src/native/external/zlib-ng/cmake/toolchain-riscv-clang.cmake Adds new RISC-V Clang toolchain configuration
src/native/external/zlib-ng/cmake/detect-intrinsics.cmake Improves AVX512VNNI intrinsics test robustness
src/native/external/zlib-ng/cmake/detect-arch.cmake Removes redundant enable_language(C) call
src/native/external/zlib-ng/arch/x86/x86_functions.h Minor formatting and comment improvements
src/native/external/zlib-ng/arch/x86/x86_features.c Reorders feature detection logic and improves comments
src/native/external/zlib-ng/arch/x86/chunkset_avx512.c Adds MSVC compiler workaround for optimization bug
src/native/external/zlib-ng/arch/s390/s390_functions.h Fixes macro definition syntax error
src/native/external/zlib-ng/arch/riscv/chunkset_rvv.c Adds safety check for negative distance values
src/native/external/zlib-ng/arch/riscv/Makefile.in Adds complete RISC-V architecture Makefile
src/native/external/zlib-ng/Makefile.in Updates version number in main Makefile
src/native/external/zlib-ng/CMakeLists.txt Improves C++ language enablement and adds Apple-specific compiler flags
src/native/external/zlib-ng/.gitignore Adds test data directories to ignore list
src/native/external/zlib-ng-version.txt Updates version reference and upstream URL

@risc-vv
Copy link

risc-vv commented Aug 6, 2025

@dotnet/samsung Could you please take a look? These changes may be related to riscv64.

@jkotas jkotas added area-System.IO.Compression and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 6, 2025
@ericstj
Copy link
Member Author

ericstj commented Aug 6, 2025

I've confirmed this fixes the performance regression in highly compressible data that we reported.

Method Job TestFile Mean Error StdDev
CompressionTest 10.0-latest+zlib-ng-2.2.5 TestDocument.pdf 275,296.5 us 6,962.04 us 20,527.74 us
CompressionTest 10.0-preview6 TestDocument.pdf 296,089.9 us 11,597.76 us 34,196.27 us
CompressionTest 9.0 TestDocument.pdf 267,746.5 us 6,403.81 us 18,881.78 us
CompressionTest 8.0 TestDocument.pdf 365,983.0 us 12,765.17 us 37,638.41 us
CompressionTest 10.0-latest+zlib-ng-2.2.5 xargs.1 226.3 us 6.87 us 20.16 us
CompressionTest 10.0-preview6 xargs.1 860.6 us 47.32 us 139.53 us
CompressionTest 9.0 xargs.1 969.4 us 56.07 us 165.32 us
CompressionTest 8.0 xargs.1 377.2 us 13.30 us 39.22 us

https://gist.github.com/ericstj/5655e50ef8c3daf22b432ee9c2d6ce56

@ericstj
Copy link
Member Author

ericstj commented Aug 6, 2025

@MihuBot benchmark System.IO.Compression

@ericstj
Copy link
Member Author

ericstj commented Aug 6, 2025

The dotnet/performance benchmark results look mostly a wash to me - which is to be expected since those didn't catch the original regression (due to small payload sizes).

@MihaZupan
Copy link
Member

For the test mentioned in dotnet/performance#4900: MihuBot/runtime-utils#1362

Method Toolchain Mean Error Ratio Allocated Alloc Ratio
Test Main 182.15 ms 0.061 ms 1.00 13.75 MB 1.00
Test PR 90.13 ms 0.027 ms 0.49 14.73 MB 1.07

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@ericstj ericstj enabled auto-merge (squash) August 7, 2025 20:38
@ericstj ericstj merged commit c2e7c99 into dotnet:main Aug 7, 2025
143 of 146 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants