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

Replace zlib with zlib-ng #16100

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Replace zlib with zlib-ng #16100

wants to merge 2 commits into from

Conversation

inetol
Copy link

@inetol inetol commented Jan 1, 2025

What does this PR do?

It was mentioned a while ago on the official Discord server about replacing the Cloudflare zlib fork with the zlib-ng one, as it already includes the changes from the CF fork, architectural and QoL improvements over the original project (the full manifesto explaining the "why" of this fork)

Supersedes #8529

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Local testing

@Jarred-Sumner
Copy link
Collaborator

Oh this is great. I was thinking about doing this

@Mrgaton
Copy link

Mrgaton commented Jan 1, 2025

I love you @inetol

@inetol inetol changed the title PoC: Replace zlib with zlib-ng Replace zlib with zlib-ng Jan 1, 2025
@inetol inetol marked this pull request as ready for review January 1, 2025 21:45
@inetol
Copy link
Author

inetol commented Jan 1, 2025

I have not seen weird things or crashes, at least on my Linux workstation. I don't know the rest of OS if there are any, please approve the workflows to better test this PR

@Jarred-Sumner
Copy link
Collaborator

Looks like it does not compile due to missing includes.

@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Jan 2, 2025

A quick benchmark on a high-end Linux x64 machine running a Next.js standalone hello world server:

Branch Requests per second
This branch 8367
Main 9155

Interestingly, individual zlib benchmarks did improve.

this branch:

clk: ~5.45 GHz
cpu: 13th Gen Intel(R) Core(TM) i9-13900
runtime: bun 1.1.42 (x64-linux)

benchmark              avg (min  max) p75   p99    (min  top 1%)
-------------------------------------- -------------------------------
deflate 12B (level 1)    14.40 µs/iter  14.25 µs      █▄
                (11.01 µs  848.15 µs)  20.63 µs ▁▁▁▃████▄▃▂▁▁▁▁▁▁▁▁▁▁
deflate 12 (level 6)     14.63 µs/iter  14.43 µs       ▄█
                (11.65 µs  826.08 µs)  18.56 µs ▁▁▁▁▁▄███▄▄▂▁▁▁▁▁▁▁▁▁
deflate 12K (level 1)    15.14 µs/iter  15.22 µs                 
                 (14.93 µs  15.40 µs)  15.27 µs ██▁▁▁▁▁█▁▁▁█▁██▁▁██▁▁
deflate 12K (level 6)    23.79 µs/iter  24.05 µs             
                 (22.66 µs  25.16 µs)  24.61 µs ██▁▁▁▁▁█▁▁█▁█▁█▁▁█▁▁▁
deflate 123K (level 1)   30.36 µs/iter  30.37 µs ███  ██     ██   
                 (29.59 µs  33.58 µs)  30.61 µs ███▁▁██▁▁█▁█▁▁██▁▁▁█▁
deflate 123K (level 6)  111.24 µs/iter 108.87 µs  ██
               (100.35 µs  522.47 µs) 195.78 µs ▁██▃▂▁▁▁▁▁▁▁▂▁▁▁▁▁▁▁▁
inflate 12               10.61 µs/iter  10.63 µs   ██
                 (10.19 µs  11.61 µs)  11.32 µs ████▁█▁▁█▁▁▁▁█▁▁▁▁▁▁▁
inflate 12K              12.61 µs/iter  12.76 µs                    
                 (12.19 µs  12.82 µs)  12.78 µs ▆▁▁▁▁▁▆▆▁▁▁▁▁▁▆▁▁▁▁█▁
inflate 123K             69.31 µs/iter  66.11 µs   
                (56.70 µs  879.39 µs) 130.28 µs ▁▅█▅▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁

main:

clk: ~5.45 GHz
cpu: 13th Gen Intel(R) Core(TM) i9-13900
runtime: bun 1.1.42 (x64-linux)

benchmark              avg (min  max) p75   p99    (min  top 1%)
-------------------------------------- -------------------------------
deflate 12B (level 1)    34.08 µs/iter  33.67 µs   ▇▄█
                (28.73 µs  830.17 µs)  48.11 µs ▁▅████▆▄▃▂▂▁▁▁▁▁▁▁▁▁▁
deflate 12 (level 6)     33.43 µs/iter  33.56 µs         
                 (32.46 µs  34.99 µs)  34.20 µs █▁▁▁██▁▁████▁█▁▁▁█▁▁▁
deflate 12K (level 1)    38.32 µs/iter  38.39 µs                  
                 (37.90 µs  39.13 µs)  38.43 µs ▆▁▁▁▁▁▆▁▁▆█▁▁▁▆▁▁▁█▁▆
deflate 12K (level 6)    46.39 µs/iter  46.47 µs          ██
                 (45.47 µs  47.38 µs)  47.08 µs █▁▁▁▁█▁▁█▁███▁▁▁▁█▁▁▁
deflate 123K (level 1)   98.72 µs/iter  94.96 µs 
                (92.80 µs  502.73 µs) 181.35 µs █▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
deflate 123K (level 6)  180.69 µs/iter 177.46 µs 
               (174.40 µs  522.59 µs) 311.00 µs █▄▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
inflate 12               10.87 µs/iter  10.74 µs        █▃
                 (6.45 µs  792.10 µs)  16.82 µs ▁▁▁▁▁▁███▆▂▂▁▁▁▁▁▁▁▁▁
inflate 12K              14.95 µs/iter  14.96 µs           
                 (14.56 µs  15.93 µs)  15.26 µs ▆▁▁▆▁▆▁▁█▆▁█▆▁▁▁▁▁▁▁▁
inflate 123K             93.85 µs/iter  87.88 µs  
                  (81.18 µs  1.13 ms) 168.78 µs ▂█▃▁▁▁▁▂▂▁▁▁▁▁▁▁▁▁▁▁▁

my guess is that inflate streaming performance is regressed a little compared to cloudflare-zlib

@inetol
Copy link
Author

inetol commented Jan 5, 2025

Looks like it does not compile due to missing includes.

Should be fixed now

my guess is that inflate streaming performance is regressed a little compared to cloudflare-zlib

IDK if it has anything to do with zlib compat layer, although the cleanest thing to do would be to refactor zlib to use the new API

@inetol inetol force-pushed the poc-zlibng branch 3 times, most recently from 4f89088 to c87ae53 Compare January 15, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants