Skip to content

curl: improve non-blocking STDIN performance #17566

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

Closed
wants to merge 2 commits into from

Conversation

denandz
Copy link
Contributor

@denandz denandz commented Jun 9, 2025

Fix adds an additional call to unpause curl, ensuring the progress function that pauses reading from STDIN is called frequently. Without this call, Curl will eventually only call the progress function once a second-ish which resulted in poor performance.

Additionally, the sleep time has been lowered to significantly improve performance (250MB/s versus 10MB/s). CPU usage remains low, with top reporting 0-1% CPU usage while waiting for input.

Fixes #17553

@denandz
Copy link
Contributor Author

denandz commented Jun 9, 2025

Some more info on how I arrived at the 1ms sleep time. Using https://github.com/denandz/httpstream2tcp and curl to proxy SSH traffic:

5ms delay:

:~/src/curl/build/src$ scp -o ProxyCommand="./curl -s --no-buffer 127.0.0.1:3000/stream -T . --no-progress-meter --expect100-timeout 0.01" ~/tmp/test.dat 127.0.0.1:weh
test.dat                                                6%  560MB  26.0MB/s   04:53 ETA

2ms delay:

scp -o ProxyCommand="./curl -s --no-buffer 127.0.0.1:3000/stream -T . --no-progress-meter --expect100-timeout 0.01" ~/tmp/test.dat 127.0.0.1:weh
test.dat                                                18% 1481MB 141.8MB/s   00:47 ETA

1ms delay:

scp -o ProxyCommand="./curl -s --no-buffer 127.0.0.1:3000/stream -T . --no-progress-meter --expect100-timeout 0.01" ~/tmp/test.dat 127.0.0.1:weh
test.dat                                                 27% 2269MB 243.5MB/s   00:24 ETA

Top output with a 1ms delay:

    PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND     
 93035 doi       20   0  238880   7328   6304 S   0.0   0.0   0:00.26 curl

@bagder
Copy link
Member

bagder commented Jun 10, 2025

I propose an even simpler take: #17569

@denandz
Copy link
Contributor Author

denandz commented Jun 10, 2025

I propose an even simpler take: #17569

Marvelous. Tested, its even faster.

scp -o ProxyCommand="./curl -s --no-buffer 127.0.0.1:3000/stream -T . --no-progress-meter --expect100-timeout 0.01" ~/tmp/test.dat 127.0.0.1:weh
test.dat                                                36% 2969MB 345.5MB/s   00:15 ETA

Happy to tweak this PR or close and go with #17569 - your call.

@denandz
Copy link
Contributor Author

denandz commented Jun 10, 2025

I managed to tease out a little more performance by moving to a select() call on non-windows platforms, returning data sooner than the 1ms delay if its available. Not sure if this is worth the additional complexity, though....

test.dat                                                              100% 8192MB 524.2MB/s   00:15

@bagder
Copy link
Member

bagder commented Jun 21, 2025

I think doing a select is fine, but this PR now has a merge conflict after the #17572 merge

@denandz
Copy link
Contributor Author

denandz commented Jun 21, 2025

I think doing a select is fine, but this PR now has a merge conflict after the #17572 merge

All good, have tweaked this PR to address the merge conflict.

@bagder
Copy link
Member

bagder commented Jun 21, 2025

"This branch cannot be rebased due to conflicts"

Please rebase onto master and force-push

@denandz denandz force-pushed the stdin-performance-fixes branch from 2bb0231 to 7fa43fa Compare June 21, 2025 22:30
@github-actions github-actions bot added tests libcurl API CI Continuous Integration labels Jun 21, 2025
@denandz denandz force-pushed the stdin-performance-fixes branch from 98af686 to 2bb0231 Compare June 21, 2025 22:49
denandz added 2 commits June 22, 2025 10:53
Using a select() call on supported platforms to check for data with
a given timeout
@denandz denandz force-pushed the stdin-performance-fixes branch from 2bb0231 to f183046 Compare June 21, 2025 22:55
@denandz
Copy link
Contributor Author

denandz commented Jun 21, 2025

"This branch cannot be rebased due to conflicts"

Please rebase onto master and force-push

Apologies, had to wrestle rebasing a bit. I think I've got this sorted now, though! :D

@bagder bagder closed this in 83baac4 Jun 25, 2025
@bagder
Copy link
Member

bagder commented Jun 25, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Performance issues with -T/--upload-file .
2 participants