-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Implement ZStandard Stream, Encoder, Decoder #119575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/native/external/zstd.patch
Outdated
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.
This should be upstreamed. You can link the PR in -version.txt file (see libunwind-version.txt). We've avoided adding patch files like these to the repo in the past.
| <UseSystemZlib Condition="'$(UseSystemZlib)' == '' and !Exists('$(IlcFrameworkNativePath)libz.a')">true</UseSystemZlib> | ||
| <!-- Use libbrotlicommon.a as the sentinel for the three brotli libs. --> | ||
| <UseSystemBrotli Condition="'$(UseSystemBrotli)' == '' and !Exists('$(IlcFrameworkNativePath)libbrotlicommon.a')">true</UseSystemBrotli> | ||
| <UseSystemZstd Condition="'$(UseSystemZstd)' == '' and !Exists('$(IlcFrameworkNativePath)libzstd.a')">true</UseSystemZstd> |
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.
UseSystemXX is paired with CLR_CMAKE_USE_SYSTEM_XX support for distros to use system installed lib. That typically happens when we are confident that distro provided version is working on major distros like Fedora and Ubuntu. See git grep CLR_CMAKE_USE_SYSTEM_BROTLI. In cases where we have many functionality-related patches to make the lib work (like llvm-libunwind has tons of local / hard-to-upstream patches: #72655), distros cannot use the system provided variant and we use the regular in-tree version.
TODOs:
- Add
CLR_CMAKE_USE_SYSTEM_ZSTDsupport in src/native/libs and VMR'srepo-projects/runtime.proj(next toCLR_CMAKE_USE_SYSTEM_BROTLI) - Test with system provided libs at least on fedora and ubuntu
- If the test fails, remove UseSystemZstd in NAOT buildintegration and file an issue to follow up
- Otherwise, check-in those (
CLR_CMAKE_USE_SYSTEM_ZSTD) changes.
|
Edit: seem to be fixed now There seems to be some problems with linux x86 build that I am unable to debug on my own. The build fails because it fails to copy libzstd.a to the coreclr build output, because it attempts to do so before the library is built. The weird part for me is that on all other builds that I checked, the "Installing: ...." steps are happening at the very end of the build, but for this particular pipeline they are happening sometime in the middle, see e.g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=1214394&view=logs&jobId=d82b64b5-3dbf-5e48-910e-ffa3eb07d628&j=d82b64b5-3dbf-5e48-910e-ffa3eb07d628&t=3aaa1282-8fdd-59be-7f23-95b71c611ad2 I think I have exhausted the more reasonable debugging Ideas, Can somebody assist here? Maybe @jkoritzinsky? Your name pops up a lot in CMakeLists git blame. |
src/libraries/System.IO.Compression.Zstandard/src/System/IO/Compression/ZstandardUtils.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression.Zstandard/src/System/IO/Compression/ZstandardUtils.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Implements #59591
TO REVIEWERS:
You can Ignore native code in src/native/external/zstd (verbatim copy of latest zstd release, included in the very first commit)
The actual C# implementation added in this PR is "only" about 4.5k LOC.
There are some patches applied to vendored zstd sources which are listed in src/native/external, neither of them should be necessary once we update to next release (maintainers didn't commit to a release date, but hopefully they will release something early next year).
Benchmarks results (taken from dotnet/performance repo) look great so far
Benchmark results