-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Migrate to zlib-ng, part 2: consume it in runtime #102403
base: main
Are you sure you want to change the base?
Conversation
To make this easier to review, it would be best to split this into 3 PRs:
|
@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor. |
The test binaries like src/native/external/zlib-ng/test/CVE-2002-0059/test.gz are going to pain to deal with for source build. Can we exclude the test subdirectory from the vendored copy? I do not expect that we are going to run the native zlib-ng tests in this repo. |
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Windows.targets
Show resolved
Hide resolved
This is better than nothing, but it is still pretty clunky experience. FWIW, I am regularly splitting my changes into multiple PRs when they are mix of large mechanical delta and smaller actual code delta. For example, I have done it a few days ago in #102422. I could have folded this mechanical change into #102392 that it is needed for, but I have intentionally split it into multiple PRs to get it signed-off and merged faster. |
As requested, I submitted #102520 to only bring in the zlib-ng code into our repo (and its licensing files), but without consuming it anywhere yet. I'll update this PR to only include the code that enables zlib-ng, and will revert the changes that remove zlib and zlib-intel (I'll remove those in a third PR). |
Big difference now: 23 files instead of 388. |
src/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated
Show resolved
Hide resolved
src/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
Outdated
Show resolved
Hide resolved
I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS. |
The intention is that we don't use system zlib anymore anywhere right? is that OK with distro maintainers? I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. |
The linked issue indicates that the repro steps require creating a new iOS project. What that tells me is that the issue was found after merging, and it was caught in servicing (8.0, back in February). I also don't see a PR that fixed the issue, it looks like a workaround was given instead. I'd like to avoid getting at that point to find out about this problem, and also avoid a workaround. Do you know if I can repro this before merging this PR? Maybe even write a test than can confirm this does not happen? |
No, the problem was in fact caused by an iOS update after we shipped so there was no way to catch it beforehand, which is why I'd like to make sure we don't have the same potential conflict. |
@@ -129,6 +129,7 @@ The .NET Foundation licenses this file to you under the MIT license. | |||
<NativeLibrary Condition="'$(_targetArchitecture)' == 'x64'" Include="$(IlcSdkPath)$(VxSortSupportName)$(LibFileExt)" /> | |||
<NativeLibrary Include="$(IlcSdkPath)$(StandaloneGCSupportName)$(LibFileExt)" /> | |||
<NativeLibrary Condition="'$(LinkStandardCPlusPlusLibrary)' != 'true' and '$(StaticICULinking)' != 'true'" Include="$(IlcSdkPath)libstdc++compat.a" /> | |||
<NativeLibrary Include="$(IlcSdkPath)libz.a" /> |
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.
Native AOT is supported for mobile. We should use the same default zlib strategy for all runtime flavors targeting given platform. I think you should introduce a property like UseSystemZLib
and use it in these target files.
(You will also want to run outer loop native aot tests to make sure that it works.)
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 assumed that you are planning to use system zlib for mobile and browser/wasm given the discussion in #101465 , but it looks like that you are planning to use system zlib for browser wasm only. What is the plan?
In any case, it would be useful to have a property to disable use of zlib-ng for mobile and native aot that advanced users can use to resolve the conflicts. This property is also going to be useful to switch to system zlib in source build scenarios.
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.
At the moment of writing my comment in the main issue, I was testing using our copy of zlib-ng everywhere. Last night I was able to get it to work for all platforms (just one CI leg failing but I think I got it fixed just now).
Since this won't make it into preview6, I was thinking I could do the check for an installed system zlib in a separate PR, as we had originally discussed it initially when I created the PR:
Thanks for your suggestions, I will make sure to keep them in mind for when I work on that.
Still in draft, please don't review yet until I get all the platforms working in CI.
Contributes to: #101465
Performance comparisons
Before: 6e43b6a (main)
After: 619b721 (from this PR)
Ran the dotnet/performance System.IO.Compression microbenchmarks.
Ubuntu 22.04 x64 (WSL)
summary: better: 38, geomean: 1.715 worse: 32, geomean: 1.038 total diff: 70Windows 11 arm64 (Surface Pro X)
summary:
better: 50, geomean: 1.362
worse: 13, geomean: 1.030
total diff: 63
macOS Monterey 12.7.5 (MacBook Pro Retina)
summary:
better: 37, geomean: 1.480
worse: 18, geomean: 1.019
total diff: 55