Uh oh!
There was an error while loading. Please reload this page.
Upgrade zlib-ng to 2.2.1 - #105771
Conversation
Tagging subscribers to this area: @dotnet/area-system-io-compression |
stephentoub
commented
Jul 31, 2024
Am I understanding correctly that this is currently using 2.1.6 from January, such that by the time .NET 9 ships it'll be almost a year old? |
Uh oh!
There was an error while loading. Please reload this page.
carlossanlop
commented
Jul 31, 2024
I started working on the zlib-ng migration before July 2nd, so the only other release available was a Release Candidate from June: https://github.com/zlib-ng/zlib-ng/releases/tag/2.2.0 In a meeting with @GrabYourPitchforks and @blowdart we also discussed that we would like to give external dependencies some time after they're released before we take them in. This is mainly why I'm asking we decide if we want to take this version, as it is only 1 month old. |
stephentoub
commented
Jul 31, 2024
Thanks. Assuming we do all the relevant due diligence, I think we should take it. Ensuring we're as up-to-date as possible makes it easier to absorb servicing changes if any arise, we've still got months before the actual release, and the currently-used version will be almost a year out-of-date by the time we release. |
@jkotas Here are my microbenchmark results testing with and without our custom allocator. I used this commit for the custom allocator removal (not yet included in this PR): https://github.com/carlossanlop/runtime/commit/828545f382cbadf5793de31819057dfef42a8f4c Summary:
DeflateDetails
GZipDetails
ZLibDetails
|
jkotas
commented
Aug 22, 2024
The custom allocator is security mitigation. It is not meant to make things faster. It is expected to make things a bit slower, but the slowdown was assumed to be in the noise range (see #84604 for the perf numbers for when it was introduced). It does not seem to be the case anymore for small payloads based on the results from the |
carlossanlop
commented
Aug 22, 2024
I understand that. I was just mentioning the notable differences between the microbenchmark results, among which one of them happened to be a tiny speed improvement, which I agree it's negligible as it is not in the range outside noise. So what is your opinion on formally including the removal of the custom allocator in this PR? I say we do it. |
jkotas
commented
Aug 22, 2024
Now that zlib-ng allocates one big memory block and manages the small memory allocations with that big block internally, the custom allocator does not provide most of the benefits that it was originally introduced for. I think it is fine to remove it. |
carlossanlop
commented
Aug 22, 2024
Ok great. BTW Seems there are some tests failing. At first glance, it seems they expect a certain file size and it's not being met anymore. I need to investigate them. |
carlossanlop
commented
Aug 23, 2024
I see what the problem is. These are the test that are failing: Test resultsIn the recently added Before that modification, the test used to pass because it was comparing the hardcoded enum values My proposed fix is to bring back the old test, and also add another test that only verifies the new |
carlossanlop
commented
Aug 23, 2024
Small errata in my previous comment: The test was not modified, it was added. The previous test still exists and is passing. So I don't need to bring it back. It's only the new test that needs to be modified to ensure the comparisons are not done between two CompressionLevel int values that are too close to each other. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jkotas
commented
Aug 23, 2024
Build breaks... |
jkotas
commented
Aug 27, 2024
#105771 (comment) from my earlier feedback is still unresolved |
| Apply https://github.com/dotnet/runtime/pull/105433.patch No newline at end of file | ||
| Also apply: | ||
| - https://github.com/dotnet/runtime/commit/ecdb625035e0e3fb7c51e908713d96d2cb2080c8.patch or cherry-pick ecdb625035e0e3fb7c51e908713d96d2cb2080c8 directly. | ||
| - https://github.com/dotnet/runtime/pull/105771.patch No newline at end of file |
There was a problem hiding this comment.
This links to a full 1000's lines bug patch. I think this should only link to a single commit with the specific change, similar to the previous line.
There was a problem hiding this comment.
Hold on, I'll have to squash everything. Sigh.
There was a problem hiding this comment.
I am sorry, this is still the wrong 1000's lines commit.
These commits should be only the zlib-ng delta vs. upstream.
There was a problem hiding this comment.
No worries, I want to do the right thing, but I am unsure how to get the correct delta that you expect.
The commit I used for the patch is the commit containing the squashed contents of this PR. That patch is showing a diff before and after updating zlib-ng (which does have a lot of changes since their last release). It's also excluding all the unnecessary files and folders, and also updating our informational json and txt files.
What am I missing?
Note: This comment thread is already old and pointing at the wrong patch (it's the patch of the PR itself). Are you looking at my most recent two commits?
There was a problem hiding this comment.
Yes, I am looking at the two most recent commits.
Take a look at #102231 for a good way to make these types of updates. Could you please update this PR to be composed of these 3 commits:
Commit 1: Verbatim copy of the zlib-ng v2.2.1 sources (with the specific directories and files excluded)
Commit 2: Our patches in zlib-ng sources. You can either keep the patches as multiple commits or they can be squashed into a single commit. Either way is fine.
Commit 3: Changes outside src/native/external/zlib-ng, including links to commit(s) 2 in zlib-ng-version.txt
It is nice to merge these PRs as "merge" instead of "squash" so that it is easier to tell what happened.
There was a problem hiding this comment.
Ok, done. If we merge this with a "merge-commit", the patch comment in zlib-ng-version.txt will be usable on the next zlib-ng update.
- docs/ - test/ - arch/s390/self-hosted-builder/
carlossanlop
commented
Sep 5, 2024
/ba-g all failures are pre-existing. The unknown one was a dead machine. |
carlossanlop
commented
Sep 5, 2024
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10723125127 |
@carlossanlop backporting to release/9.0 failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Update to zlib-ng 2.2.1, excluding the folders: - docs/ - test/ - arch/s390/self-hosted-builder/
Applying: Apply slide_hash and deflate patch with casts and asserts.
Applying: Remove custom allocator.
Using index info to reconstruct a base tree...
M src/native/libs/System.IO.Compression.Native/CMakeLists.txt
M src/native/libs/System.IO.Compression.Native/zlib_allocator_unix.c
Falling back to patching base and 3-way merge...
Removing src/native/libs/System.IO.Compression.Native/zlib_allocator_win.c
CONFLICT (modify/delete): src/native/libs/System.IO.Compression.Native/zlib_allocator_unix.c deleted in Remove custom allocator. and modified in HEAD. Version HEAD of src/native/libs/System.IO.Compression.Native/zlib_allocator_unix.c left in tree.
Removing src/native/libs/System.IO.Compression.Native/zlib_allocator.h
Auto-merging src/native/libs/System.IO.Compression.Native/CMakeLists.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0003 Remove custom allocator.
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
@carlossanlop an error occurred while backporting to release/9.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Stable version 2.2.1 was released on July 2nd (a month ago).
Let's discuss if we want to include this update in .NET 9 or wait until after main points to .NET 10.
I am following the instructions we're adding for native dependency updates, to see if there's anything special that needs to be added: #105045 . For example: I decided to update the THIRD_PARTY_NOTICES.TXT to match the license exactly as it shows up in the 2.2.1 release commit, not on the
developbranch, as we don't know if the license would change from one version to another. The contents are the same except for some line breaks.