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

Add "no_wait" mode for file synchronization #1425

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

haze518
Copy link
Collaborator

@haze518 haze518 commented Jan 7, 2025

This PR introduces a new "no_wait" mode to the file synchronization process. The "nowait" mode allows file operations to proceed without waiting for certain synchronization steps to complete, improving performance in scenarios where blocking is unnecessary

@haze518 haze518 requested review from spetz, hubcio and numinnex January 7, 2025 15:07
@haze518 haze518 marked this pull request as draft January 7, 2025 16:15
@haze518 haze518 force-pushed the persister_via_channels_last branch 3 times, most recently from dbd5f09 to 1f65bbe Compare January 8, 2025 04:46
@coveralls
Copy link
Collaborator

coveralls commented Jan 8, 2025

Pull Request Test Coverage Report for Build 12687277738

Details

  • 214 of 243 (88.07%) changed or added relevant lines in 24 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.05%) to 75.37%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server/src/streaming/streams/storage.rs 2 4 50.0%
server/src/streaming/topics/storage.rs 2 4 50.0%
server/src/streaming/storage.rs 5 8 62.5%
server/src/streaming/segments/storage.rs 7 14 50.0%
server/src/streaming/persistence/task.rs 63 78 80.77%
Files with Coverage Reduction New Missed Lines %
server/src/configs/system.rs 1 85.0%
Totals Coverage Status
Change from base Build 12639344475: 0.05%
Covered Lines: 24343
Relevant Lines: 32298

💛 - Coveralls

@haze518 haze518 force-pushed the persister_via_channels_last branch from 1f65bbe to 52cfd04 Compare January 8, 2025 10:18
@haze518 haze518 marked this pull request as ready for review January 8, 2025 10:31
@spetz
Copy link
Collaborator

spetz commented Jan 8, 2025

Great job, I can certainly see the improvements with nowait mode (as expected). I've just executed a quick few tests on M1 Max, and here are the example results for the following benchmark:

cargo r --bin iggy-bench -r send --streams 10 --producers 10 --message-size 1000 --messages-per-batch 1000 --message-batches 3000 tcp

nowait

Results: Total throughput: 1966.74 MB/s, 1920640 messages/s, average throughput: 196.67 MB/s, average p50 latency: 4.22 ms, average p90 latency: 5.13 ms, average p95 latency: 5.48 ms, average p99 latency: 6.83 ms, average p999 latency: 255.91 ms, average latency: 5.21 ms, median latency: 4.22 ms

wait

Results: Total throughput: 1671.96 MB/s, 1632774 messages/s, average throughput: 167.20 MB/s, average p50 latency: 3.77 ms, average p90 latency: 4.99 ms, average p95 latency: 6.06 ms, average p99 latency: 20.11 ms, average p999 latency: 589.00 ms, average latency: 6.13 ms, median latency: 3.77 ms

I will do a few more tests tomorrow on Linux PC, but it looks like it could be merged already :)

Copy link
Collaborator

@spetz spetz left a comment

Choose a reason for hiding this comment

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

Looks good, and works even better :) Please update the Cargo.toml in server to 0.4.100 so that it will be automatically released.

sdk/src/confirmation.rs Outdated Show resolved Hide resolved
@spetz
Copy link
Collaborator

spetz commented Jan 9, 2025

And a quick update after running the same benchmark for nowait mode on Linux PC (7950X, 64GB RAM, Gen 4 NVMe).

Results: Total throughput: 3212.28 MB/s, 3136991 messages/s, average throughput: 321.23 MB/s, average p50 latency: 2.66 ms, average p90 latency: 5.14 ms, average p95 latency: 6.22 ms, average p99 latency: 8.73 ms, average p999 latency: 12.16 ms, average latency: 3.19 ms, median latency: 2.66 ms

@haze518 haze518 changed the title Add "nowait" mode for file synchronization Add "no_wait" mode for file synchronization Jan 9, 2025
Copy link
Collaborator

@hubcio hubcio left a comment

Choose a reason for hiding this comment

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

LGTM

@haze518 haze518 merged commit 07bf86b into master Jan 9, 2025
15 checks passed
@haze518 haze518 deleted the persister_via_channels_last branch January 9, 2025 09:54
@hubcio
Copy link
Collaborator

hubcio commented Jan 9, 2025

like promised, some charts, master:
image

no_wait:
image

@haze518
Copy link
Collaborator Author

haze518 commented Jan 9, 2025

like promised, some charts, master: image

no_wait: image

Yes, now I can see the difference :) When I was testing, I was focusing on the median and the message throughput rather than the 90+ percentiles. Thanks!

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.

4 participants