Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

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.

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?

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?

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.

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:

• We got slightly faster results when removing the custom allocator.
• There were no cases where keeping the custom allocator was faster.
• There was no change in the allocated and allocation ratio columns.

Deflate

GZip

ZLib

There were no cases where keeping the custom allocator was faster.

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 sum micro-benchmark.

The custom allocator is security mitigation. It is not meant to make things faster.

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.

So what is your opinion on formally including the removal of the custom allocator in this PR? I say we do it.

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.

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.

I see what the problem is. These are the test that are failing:

===========================================================================================================
    Discovering: System.IO.Compression.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.IO.Compression.Tests (found 281 of 293 test cases)
    Starting:    System.IO.Compression.Tests (parallel test collections = on [16 threads], stop on fail = off)
      System.IO.Compression.ZLibStreamUnitTests.ZLibCompressionLevel_SizeInOrder(testFile: "UncompressedTestFiles\\TestDocument.doc") [FAIL]
        Expected 6773 <= 6768 for quality 6
        Stack Trace:
          C:\Users\calope\source\repos\runtime\src\libraries\Common\tests\System\IO\Compression\CompressionStreamUnitTestBase.cs(558,0): at System.IO.Compression.CompressionStreamUnitTestBase.CompressionLevel_SizeIn 
  OrderBase(String testFile)
          C:\Users\calope\source\repos\runtime\src\libraries\System.IO.Compression\tests\CompressionStreamUnitTests.ZLib.cs(158,0): at System.IO.Compression.ZLibStreamUnitTests.ZLibCompressionLevel_SizeInOrder(Strin 
  g testFile)
          --- End of stack trace from previous location ---
      System.IO.Compression.DeflateStreamUnitTests.ZLibCompressionLevel_SizeInOrder(testFile: "UncompressedTestFiles\\TestDocument.doc") [FAIL]
        Expected 6771 <= 6766 for quality 6
        Stack Trace:
          C:\Users\calope\source\repos\runtime\src\libraries\Common\tests\System\IO\Compression\CompressionStreamUnitTestBase.cs(558,0): at System.IO.Compression.CompressionStreamUnitTestBase.CompressionLevel_SizeIn 
  OrderBase(String testFile)
          C:\Users\calope\source\repos\runtime\src\libraries\System.IO.Compression\tests\CompressionStreamUnitTests.Deflate.cs(227,0): at System.IO.Compression.DeflateStreamUnitTests.ZLibCompressionLevel_SizeInOrder 
  (String testFile)
          --- End of stack trace from previous location ---
      System.IO.Compression.GzipStreamUnitTests.ZLibCompressionLevel_SizeInOrder(testFile: "UncompressedTestFiles\\TestDocument.doc") [FAIL]
        Expected 6781 <= 6776 for quality 6
        Stack Trace:
          C:\Users\calope\source\repos\runtime\src\libraries\Common\tests\System\IO\Compression\CompressionStreamUnitTestBase.cs(558,0): at System.IO.Compression.CompressionStreamUnitTestBase.CompressionLevel_SizeIn 
  OrderBase(String testFile)
          C:\Users\calope\source\repos\runtime\src\libraries\System.IO.Compression\tests\CompressionStreamUnitTests.Gzip.cs(449,0): at System.IO.Compression.GzipStreamUnitTests.ZLibCompressionLevel_SizeInOrder(Strin 
  g testFile)
          --- End of stack trace from previous location ---
    Finished:    System.IO.Compression.Tests
  === TEST EXECUTION SUMMARY ===
     System.IO.Compression.Tests  Total: 956, Errors: 0, Failed: 3, Skipped: 0, Time: 11.256s
  ----- end 2024-08-22 16:47:49.04 ----- exit code 1 ----------------------------------------------------------

In the recently added ZLibCompressionOptions from #105430, this test was modified to compare each compression level int value with the next one. Unfortunately, I don't think we should try to guarantee file sizes between one number and the immediate next one.

Before that modification, the test used to pass because it was comparing the hardcoded enum values CompressionLevel.NoCompression|Fastest|Optimal|Smallest, whose underlying compression level int values are far enough that we can actually guarantee that the file sizes will be as expected in the tests:
https://github.com/dotnet/runtime/blame/7a67973961ea1b997456ac76a7a5df0503354c5c/src/libraries/Common/tests/System/IO/Compression/CompressionStreamUnitTestBase.cs#L475-L495

My proposed fix is to bring back the old test, and also add another test that only verifies the new ZLibCompressionOptions using the same underlying int values used by the old test.

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.

Build breaks...

#105771 (comment) from my earlier feedback is still unresolved

/ba-g all failures are pre-existing. The unknown one was a dead machine.

/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 128

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