Skip to content

Revert "Remove custom allocator." #117949

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

Merged
merged 7 commits into from
Aug 1, 2025

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jul 22, 2025

This reverts commit c62bc5b.

We had reports from a partner that they were seeing poor performance from .NET 9.0 that pointed to heap allocation in zlib.

| Name                                                           	Inc %	     Inc	Exc %	   Exc |
|  + system.io.compression.native!CompressionNative_DeflateInit2_	49.6	  52,302	 0.0	    37 |
|   + system.io.compression.native!deflateInit2                  	49.5	  52,130	 0.0	    18 |
|   |+ system.io.compression.native!alloc_deflate                	48.6	  51,218	 0.0	    39 |
|   ||+ module ucrtbase <<ucrtbase!_aligned_malloc>>             	48.1	  50,715	 0.0	    29 |
|   |||+ ntdll!RtlpAllocateHeapInternal                          	48.1	  50,686	 0.0	    23 |
|   ||| + ntdll!RtlpAllocateHeap                                 	48.1	  50,663	45.9	48,399 |

This was caused by native heap fragmentation. We tested the partner's service with a fix that added back the zlib custom allocator and it resolved the issue. We shipped 9.0 preview 7 and RC1 with the allocator, it was removed in RC2. This is a clean revert of the commit that removed the allocator.

We intend to ask for this fix for servicing in .NET 9.0 once it's in.

@Copilot Copilot AI review requested due to automatic review settings July 22, 2025 20:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR reverts the removal of the custom allocator for zlib to address performance regression in .NET 9.0. A partner reported poor performance caused by native heap fragmentation when using the standard process heap for zlib allocations.

  • Reintroduces custom allocator implementation with defense-in-depth security features
  • Adds platform-specific allocator implementations for Windows and Unix systems
  • Integrates custom allocator into zlib initialization

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
zlib_allocator_win.c Windows-specific custom allocator with separate heap and security cookies
zlib_allocator_unix.c Unix-specific custom allocator using malloc/calloc with security validation
zlib_allocator.h Header file declaring the custom allocator function signatures
pal_zlib.c Integration point to use custom allocator in zlib stream initialization
CMakeLists.txt Build configuration to include platform-appropriate allocator source file

Copy link
Contributor

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

@ericstj
Copy link
Member Author

ericstj commented Jul 23, 2025

Performance results from this change compared to other iterations. Here Allocator is the initial version of this change, minAllocator is the current version of the change (including removal of zeroing and canary checks).

Method Job Mean Error StdDev
Test 9.0 304.1 ms 6.07 ms 15.00 ms
Test 9.0+Allocator 291.9 ms 5.83 ms 14.08 ms
Test 9.0+minAllocator 267.8 ms 4.56 ms 4.04 ms
Test 10.0 304.0 ms 7.97 ms 23.26 ms
Test 10.0+minAllocator 275.3 ms 5.39 ms 9.30 ms

@ericstj
Copy link
Member Author

ericstj commented Aug 1, 2025

/backport to release/9.0-staging

Copy link
Contributor

github-actions bot commented Aug 1, 2025

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/16680374285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants