Skip to content
This repository has been archived by the owner on Mar 27, 2023. It is now read-only.

Fix severe compression bug introduced by #97 #99

Merged
merged 8 commits into from
Sep 17, 2022
Merged

Fix severe compression bug introduced by #97 #99

merged 8 commits into from
Sep 17, 2022

Conversation

henry40408
Copy link
Contributor

@henry40408 henry40408 commented Sep 15, 2022

closes #98

How to reproduce

A error like curl: (23) Failed reading the chunked-encoded stream will occur and only first 4KB content of file will be returned.

Root cause

sfz/src/server/serve.rs

Lines 416 to 419 in 70b431a

.map(|b| match b {
Ok(b) => compress(&b, content_encoding),
Err(e) => Err(e),
});

Maybe the body stream isn't split like how I expected, so only the first item of stream get compressed and returned then the rest are dropped. Since the buffer of file stream is 4KB, it might explain why only the first 4KB will be returned:

let mut buf = BytesMut::zeroed(4_096);

Solution

brotli level = 6 level = 0, fastest
benchmark Running 10s test @ http://localhost:5000/Cargo.toml
2 threads and 10 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 5.45ms 2.20ms 23.10ms 72.66%
Req/Sec 0.93k 79.59 1.06k 70.00%
18434 requests in 10.01s, 18.09MB read
Requests/sec: 1841.70
Transfer/sec: 1.81MB
Running 10s test @ http://localhost:5000/Cargo.toml
2 threads and 10 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 2.97ms 1.09ms 19.27ms 69.77%
Req/Sec 1.70k 110.24 3.07k 96.52%
33932 requests in 10.10s, 38.96MB read
Requests/sec: 3359.58
Transfer/sec: 3.86MB
response size (1561) 700 (44%) 875 (56%)
brotli level = 6 level = 0, fastest
benchmark Running 10s test @ http://localhost:5000/1342-0.txt
2 threads and 10 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 1.30s 432.24ms 1.79s 76.92%
Req/Sec 4.71 3.89 20.00 78.57%
68 requests in 10.01s, 16.04MB read
Socket errors: connect 0, read 0, write 0, timeout 3
Requests/sec: 6.79
Transfer/sec: 1.60MB
Running 10s test @ http://localhost:5000/1342-0.txt
2 threads and 10 connections
Thread Stats Avg Stdev Max +/- Stdev
Latency 880.53ms 337.25ms 1.51s 68.52%
Req/Sec 6.25 4.69 30.00 89.47%
108 requests in 10.01s, 42.28MB read
Requests/sec: 10.78
Transfer/sec: 4.22MB
response size (798774) 246517 (30%) 405123 (50%)

@henry40408 henry40408 marked this pull request as ready for review September 15, 2022 16:30
Copy link
Owner

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Nice rework! I am fine with lowering brotli compression level.

Just some nits, nothing prominent so far. Thank you!

src/http/content_encoding.rs Show resolved Hide resolved
src/http/content_encoding.rs Outdated Show resolved Hide resolved
src/http/content_encoding.rs Outdated Show resolved Hide resolved
Copy link
Owner

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thank you for getting this job done!

@weihanglo weihanglo merged commit 5055c0f into weihanglo:master Sep 17, 2022
@henry40408 henry40408 deleted the feature/issue-98 branch September 17, 2022 12:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve brotli compression performance
2 participants