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

Increase chunk size from 4KB to 16KB to fix performance issue. #225

Merged
merged 5 commits into from
May 5, 2024

Conversation

HuakunShen
Copy link
Contributor

Python and golang implementations have 16KB chunk size but the rust version has 4KB.

I experienced a slower speed when sending files from a MacBook with M1 pro chip.

Looks like the issue is chunk size. Once I update chunk size to 16KB the speed aligns with the python and golang implementations.

For more information, see issue #224

I found the potential issue. I reviewed the code of the python and golang version with a debugger and found that the rust implementation has a different chunk size for each send. Python and Golang implementations both send 16KB at a time while the rust version sends 4KB at a time.

After setting chunk size to 16KB I get full speed.

Python

File sending starts from here https://github.com/magic-wormhole/magic-wormhole/blob/02407c4aa4cc3f8d8cd01d549fdc72a5f5d77010/> src/wormhole/cli/cmd_send.py#L442

fs = basic.FileSender()

with self._timing.add("tx file"):
    with progress:
        if filesize:
            # don't send zero-length files
            yield fs.beginFileTransfer(
                self._fd_to_send,
                record_pipe,
                transform=_count_and_hash)

The chunk size is defined in twisted package twisted.protocols.basic.FileSender

File chunks are read here https://github.com/twisted/twisted/blob/02a2b658cd1ade5d7f41f97d898913686313e615/src/twisted/> protocols/basic.py#L892

CHUNK_SIZE is a constant defined as CHUNK_SIZE = 2**14 (which is 16384bytes, 16kB)
at https://github.com/twisted/twisted/blob/02a2b658cd1ade5d7f41f97d898913686313e615/src/twisted/protocols/basic.py#L857

Golang

The golang implementation also defines chunk size to be 16KB (recordSize := (1 << 14))

See https://github.com/psanford/wormhole-william/blob/68dc3447a8585b060fb1e6836a23847700ab9207/wormhole/send.go#L363

Rust

Rust is using 4KB.

let mut plaintext = Box::new([0u8; 4096]);

I changed 4096 to 16384 and build a release build that gives me 117MB/s when sender is M1 pro Mac.

I think the rust implementation can also use 16KB.

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.29%. Comparing base (6082d8b) to head (40c305b).

❗ Current head 40c305b differs from pull request most recent head 619a366. Consider uploading reports for the commit 619a366 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   39.24%   39.29%   +0.04%     
==========================================
  Files          18       18              
  Lines        3088     3092       +4     
==========================================
+ Hits         1212     1215       +3     
- Misses       1876     1877       +1     

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

Copy link
Collaborator

@felinira felinira left a comment

Choose a reason for hiding this comment

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

Interesting findings, seems like a good idea to align this with other implementations.

src/transfer/v1.rs Outdated Show resolved Hide resolved
Co-authored-by: Fina <code@felinira.net>
@felinira felinira merged commit 420f152 into magic-wormhole:master May 5, 2024
12 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

4 participants