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

assets: add support for configurable bufsize for FileHandler #649

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

jpelkonen
Copy link
Contributor

The current implementation is using the blocksize as buffer size. In many use cases, the size is too small and limiting throughput. This commit adds an new option, buffer_size in the FileOptions, which, when set, overrides the default value.

Also included is a cargo criterion benchmark, which can be run with

cargo bench --bench file_handler

The benchmark shows major performance improvements with larger buffers when serving large files. The below results show that on my system (Apple MacBook Pro M2 Max) the throughput improves by an order of magnitude just by increasing the buf_size to 128k when serving a larger file (16MB).

server_bench/test_file_handler/filesize: 16777216, bufsize: None
                        time:   [58.284 ms 59.769 ms 61.381 ms]
                        thrpt:  [260.67 MiB/s 267.70 MiB/s 274.52 MiB/s]
                 change:
                        time:   [-3.4531% +0.1903% +3.8002%] (p = 0.92 > 0.05)
                        thrpt:  [-3.6610% -0.1899% +3.5766%]
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  6 (6.00%) high mild
  4 (4.00%) high severe
server_bench/test_file_handler/filesize: 16777216, bufsize: Some(131072)
                        time:   [7.0873 ms 7.2185 ms 7.3694 ms]
                        thrpt:  [2.1202 GiB/s 2.1646 GiB/s 2.2046 GiB/s]
                 change:
                        time:   [-0.8280% +2.2019% +5.3682%] (p = 0.16 > 0.05)
                        thrpt:  [-5.0947% -2.1545% +0.8350%]
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  3 (3.00%) high mild
  7 (7.00%) high severe
 

The current implementation is using the blocksize as buffer size.
In many use cases, the size is too small and limiting throughput.
This commit adds an new option, buffer_size in the FileOptions, which,
when set, overrides the default value.

Also included is a cargo criterion benchmark, which can be run with

```
cargo bench --bench file_handler
```

The benchmark shows major performance improvements with larger buffers
when serving large files.

Signed-off-by: Janne Pelkonen <jpelkonen@ideanovatech.com>
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 78.27%. Comparing base (f6e467e) to head (8f2b9f0).

❗ Current head 8f2b9f0 differs from pull request most recent head a693c2d. Consider uploading reports for the commit a693c2d to get more accurate results

Files Patch % Lines
gotham/src/handler/assets/mod.rs 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
- Coverage   78.49%   78.27%   -0.22%     
==========================================
  Files          72       72              
  Lines        2088     2090       +2     
==========================================
- Hits         1639     1636       -3     
- Misses        449      454       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Janne Pelkonen <jpelkonen@ideanovatech.com>
Copy link
Member

@msrd0 msrd0 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I ran your benchmark, and I can confirm your findings (Ryzen 7840HS):

server_bench/test_file_handler/filesize: 1024, bufsize: Some(131072)
                        time:   [43.396 ms 43.458 ms 43.527 ms]
                        thrpt:  [22.974 KiB/s 23.011 KiB/s 23.044 KiB/s]
Found 14 outliers among 100 measurements (14.00%)
  14 (14.00%) high severe
server_bench/test_file_handler/filesize: 1024, bufsize: None
                        time:   [43.405 ms 43.491 ms 43.596 ms]
                        thrpt:  [22.938 KiB/s 22.993 KiB/s 23.039 KiB/s]
Found 15 outliers among 100 measurements (15.00%)
  1 (1.00%) low mild
  14 (14.00%) high severe

server_bench/test_file_handler/filesize: 131072, bufsize: Some(131072)
                        time:   [120.68 µs 122.37 µs 124.21 µs]
                        thrpt:  [1006.3 MiB/s 1021.5 MiB/s 1.0115 GiB/s]
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high severe
server_bench/test_file_handler/filesize: 131072, bufsize: None
                        time:   [31.714 ms 33.428 ms 35.096 ms]
                        thrpt:  [3.5617 MiB/s 3.7393 MiB/s 3.9415 MiB/s]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

server_bench/test_file_handler/filesize: 16777216, bufsize: Some(131072)
                        time:   [8.0415 ms 8.4478 ms 8.8735 ms]
                        thrpt:  [1.7609 GiB/s 1.8496 GiB/s 1.9430 GiB/s]
server_bench/test_file_handler/filesize: 16777216, bufsize: None
                        time:   [74.632 ms 77.415 ms 80.045 ms]
                        thrpt:  [199.89 MiB/s 206.68 MiB/s 214.39 MiB/s]

@msrd0 msrd0 merged commit 82b5a03 into gotham-rs:main Apr 10, 2024
6 of 9 checks passed
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.

None yet

2 participants