Skip to content
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

Merged
merged 39 commits into from
Jul 3, 2024

Conversation

carlossanlop
Copy link
Member

@carlossanlop carlossanlop commented May 17, 2024

Contributes to: #101465

  • Copied latest zlib-ng release and updated license-related files: Migrate to zlib-ng, part 1: copy source code only #102520
  • Updated cmake code
  • Confirmed full local build on Linux passed
  • Confirmed full local build on Windows passed (including NativeAOT)
  • Confirmed full local build on MacOS passed
  • Tests pass locally on Linux and Windows (System.IO.Compression, ZipFile, Brotli, System.IO.Packaging)
  • Fix MacOS issues in CI
  • Fix some quick temporary workarounds
  • Compare performance before and after on Linux and Windows, on x64 and arm64
  • If zlib-ng is found installed in the current distro, use that instead.
  • Deleted zlib and zlib-intel references (source code will be deleted in the next PR).
  • Run runtime-community and fix any issues
  • Run runtime-nativeaot-outerloop and fix any issues
  • Run runtime-extra-platforms and fix any issues
  • Adjust levels if necessary to compensate any regressions.

Performance comparisons

dotnet/performance benchmark results: #102403 (comment)

Comparison of CompressionLevel file sizes and elapsed time: #102403 (comment)

@jkotas
Copy link
Member

jkotas commented May 17, 2024

To make this easier to review, it would be best to split this into 3 PRs:

  • Copied latest zlib-ng release and updated license-related file
  • All the other steps
  • Deleted zlib and zlib-intel references

@carlossanlop
Copy link
Member Author

@jkotas yes, I can do that. I don't want you to keep a fire extinguisher next to your overworked processor.

@carlossanlop
Copy link
Member Author

To make this easier to review, it would be best to split this into 3 PRs:

@jkotas I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0

@jkotas
Copy link
Member

jkotas commented May 21, 2024

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.

@jkotas
Copy link
Member

jkotas commented May 21, 2024

To make this easier to review, it would be best to split this into 3 PRs:

I force-pushed 3 commits to make this easier to review. This is the commit with all the changes that coupled zlib-ng with runtime: c65dad0

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.

@carlossanlop
Copy link
Member Author

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).

@carlossanlop carlossanlop changed the title Migrate to zlib-ng Migrate to zlib-ng, part 2: consume it in runtime May 21, 2024
@carlossanlop
Copy link
Member Author

Big difference now: 23 files instead of 388.

src/native/external/zlib-intel.cmake Outdated Show resolved Hide resolved
src/native/external/zlib-ng.cmake Outdated Show resolved Hide resolved
eng/native/configurecompiler.cmake Outdated Show resolved Hide resolved
@carlossanlop
Copy link
Member Author

I updated the PR description to include some initial perf comparisons in Linux, Windows and MacOS.

src/mono/mono/mini/CMakeLists.txt Outdated Show resolved Hide resolved
src/mono/mono/mini/CMakeLists.txt Outdated Show resolved Hide resolved
src/mono/mono/mini/CMakeLists.txt Outdated Show resolved Hide resolved
src/native/libs/System.IO.Compression.Native/pal_zlib.c Outdated Show resolved Hide resolved
@akoeplinger
Copy link
Member

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. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)

@carlossanlop
Copy link
Member Author

I wonder if we should set ZLIB_SYMBOL_PREFIX to e.g. dotnet_ so we don't run into conflicting symbols like what happened with ICU on iOS recently (#98941)

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?

@akoeplinger
Copy link
Member

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.

@carlossanlop
Copy link
Member Author

/azp run runtime-community

This comment was marked as outdated.

@carlossanlop
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

This comment was marked as outdated.

@carlossanlop
Copy link
Member Author

/azp run runtime-extra-platforms

This comment was marked as outdated.

@carlossanlop
Copy link
Member Author

/ba-g all failures have been investigated, none are related to this PR

@carlossanlop carlossanlop merged commit f5c9a5e into dotnet:main Jul 3, 2024
210 of 229 checks passed
@carlossanlop carlossanlop deleted the zlib-ng branch July 3, 2024 06:26
@carlossanlop
Copy link
Member Author

This was reverted because it broke the official build. I'll take a look soon.

@carlossanlop carlossanlop restored the zlib-ng branch July 4, 2024 17:29
carlossanlop added a commit to carlossanlop/runtime that referenced this pull request Jul 4, 2024
@carlossanlop carlossanlop deleted the zlib-ng branch July 5, 2024 20:44
carlossanlop added a commit that referenced this pull request Jul 8, 2024
…104454)

* Reapply "Migrate to zlib-ng, part 2: consume it in runtime (#102403)" (#104414)
* Apply jkotas comment suggestion in configureplatform.cmake
* Delete unnecessary comment in zlib-ng.cmake
* Fix windows nativeaot failure happening when executing:

build.cmd -ci -arch x64 -os windows  -s clr.nativeaotlibs+clr.nativeaotruntime+libs+packs -c Release /p:BuildNativeAOTRuntimePack=true /p:SkipLibrariesNativeRuntimePackages=true
@github-actions github-actions bot locked and limited conversation to collaborators Aug 5, 2024
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.