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

Implement zero copy writes for the WebSocket writer #9634

Closed
wants to merge 15 commits into from

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Nov 3, 2024

Implement zero copy writes for the WebSocket writer
closes #9633

python/cpython#91166 made writelines zero copy for Python 3.12+. For older versions it will still join the bytes

Would like another set of 👀 on this to make sure it makes sense.

@bdraco bdraco added backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot labels Nov 3, 2024
Copy link

codspeed-hq bot commented Nov 3, 2024

CodSpeed Performance Report

Merging #9634 will degrade performances by 16.77%

Comparing zeroconf_writes (95efb03) with master (5b654d5)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 11 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master zeroconf_writes Change
test_one_thousand_large_round_trip_websocket_text_messages[pyloop] 22.8 ms 27.4 ms -16.77%
test_send_one_hundred_websocket_text_messages_with_mask[pyloop] 807.9 µs 745.5 µs +8.36%

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 3, 2024
Copy link

codecov bot commented Nov 3, 2024

❌ 6 Tests Failed:

Tests completed Failed Passed Skipped
3378 6 3372 24
View the top 3 failed tests by shortest run time
tests.test_client_ws_functional test_heartbeat_connection_closed[pyloop]
Stack Traces | 0.397s run time
No failure message available
tests.test_client_ws_functional test_heartbeat[pyloop]
Stack Traces | 10.1s run time
No failure message available
tests.test_client_ws_functional test_heartbeat_no_pong_concurrent_receive[pyloop]
Stack Traces | 10.2s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@bdraco bdraco changed the title Implement zerocopy writes for the WebSocket writer Implement zero copy writes for the WebSocket writer Nov 3, 2024
@bdraco
Copy link
Member Author

bdraco commented Nov 3, 2024

Benchmarks barely show any improvement. I think the message payload would have to be rather large for it to matter.

We don't have any benchmarks for that

@bdraco bdraco marked this pull request as ready for review November 3, 2024 04:07
@bdraco bdraco marked this pull request as draft November 3, 2024 06:13
@bdraco
Copy link
Member Author

bdraco commented Nov 3, 2024

Still probably not a good benchmark. Will need to extend the round trip WebSocket benchmark to have another one that sends large messages to really get an idea of how it will perform

@bdraco
Copy link
Member Author

bdraco commented Nov 3, 2024

Its looking like this is worse because the reader is more fragmented. Probably worth optimizing the reading of larger messages before this would make sense

@bdraco bdraco closed this Nov 3, 2024
@bdraco bdraco deleted the zeroconf_writes branch November 3, 2024 07:43
@bdraco bdraco restored the zeroconf_writes branch November 3, 2024 16:08
@bdraco bdraco reopened this Nov 3, 2024
@bdraco
Copy link
Member Author

bdraco commented Nov 3, 2024

I think part of the reason it’s slower is the protocol gets paused. Everything backs up against the drain waiter in the writer anyways so maybe we need a queue that doesn’t pause writes in two places

Nevermind. I was misremembering. It’s reads that get paused which makes more sense

@bdraco
Copy link
Member Author

bdraco commented Nov 3, 2024

Anyways. Let’s see what the profile looks like with now with all the memcpy reduction optimizations

@bdraco
Copy link
Member Author

bdraco commented Nov 3, 2024

We still end up with more fragmentation somehow.

Probably need to work on the reader some more

@bdraco bdraco closed this Nov 3, 2024
@bdraco bdraco deleted the zeroconf_writes branch November 3, 2024 16:35
@bdraco bdraco restored the zeroconf_writes branch November 5, 2024 05:24
@bdraco bdraco reopened this Nov 5, 2024
@bdraco
Copy link
Member Author

bdraco commented Nov 5, 2024

need to look at this with syscalls

@bdraco bdraco closed this Nov 5, 2024
@bdraco bdraco deleted the zeroconf_writes branch November 11, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.10 Trigger automatic backporting to the 3.10 release branch by Patchback robot backport-3.11 Trigger automatic backporting to the 3.11 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Look into zero copy writes for the websocket writer
1 participant