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

transfer: limit Windows SO_SNDBUF updates to once a second #10611

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Feb 26, 2023

  • Change readwrite_upload() to call win_update_buffer_size() no more than once a second to update SO_SNDBUF (send buffer limit).

Prior to this change during an upload readwrite_upload() could call win_update_buffer_size() anywhere from hundreds of times per second to an extreme test case of 100k per second (which is likely due to a bug, and that will be reported separately). In the latter case WPA profiler showed win_update_buffer_size was the highest capture count in readwrite_upload. In any case the calls were excessive and unnecessary.

Closes #xxxx

- Change readwrite_upload() to call win_update_buffer_size() no more
  than once a second to update SO_SNDBUF (send buffer limit).

Prior to this change during an upload readwrite_upload() could call
win_update_buffer_size() anywhere from hundreds of times per second to
an extreme test case of 100k per second (which is likely due to a bug,
and that will be reported separately). In the latter case WPA profiler
showed win_update_buffer_size was the highest capture count in
readwrite_upload. In any case the calls were excessive and unnecessary.

Closes #xxxx
@jay jay added performance Windows Windows-specific labels Feb 26, 2023
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Sounds very reasonable!

@icing
Copy link
Contributor

icing commented Feb 27, 2023

Could we move this into the cf-socket.c and keep the update time in its context? I think this would be a better place where the actual network writes happen.

#if defined(WIN32) && defined(USE_WINSOCK)
struct curltime last_sndbuf_update; /* last time readwrite_upload called
win_update_buffer_size */
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Try putting entries in (roughly) size order in the struct to allow better packing optimization. Ie you could add this new struct above the two unsigned char entries.

@jay
Copy link
Member Author

jay commented Feb 28, 2023

Could we move this into the cf-socket.c

It was in readwrite_upload so it's only used on sockets that are being used by libcurl to send (large) request uploads

@jay jay closed this in 0b84d0c Mar 1, 2023
@jay jay deleted the win_limit_so_sndbuf_updates branch March 1, 2023 06:31
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
- Change readwrite_upload() to call win_update_buffer_size() no more
  than once a second to update SO_SNDBUF (send buffer limit).

Prior to this change during an upload readwrite_upload() could call
win_update_buffer_size() anywhere from hundreds of times per second to
an extreme test case of 100k per second (which is likely due to a bug,
see curl#10618). In the latter case WPA profiler showed
win_update_buffer_size was the highest capture count in
readwrite_upload. In any case the calls were excessive and unnecessary.

Ref: curl#2762

Closes curl#10611
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

3 participants