Implement zerocopy writes #990
Conversation
CodSpeed Performance ReportMerging #990 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 2671 2673 +2
=========================================
+ Hits 2671 2673 +2 ☔ View full report in Codecov by Sentry. |
|
|
With Python 3.12+ and later transport.writelines is implemented as `sendmsg(..., IOV_MAX)` which allows us to avoid joining the bytes and sending them in one go
With Python 3.12+ and later transport.writelines is implemented as `sendmsg(..., IOV_MAX)` which allows us to avoid joining the bytes and sending them in one go
bdraco
force-pushed
the
zerocopy_writes
branch
from
0b78193
to
35cf0bd
Compare
WalkthroughThe changes in this pull request primarily involve renaming and modifying the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
aioesphomeapi/_frame_helper/base.py (1)
186-196: Excellent implementation of zerocopy writes.The changes effectively implement zerocopy writes by:
- Accepting an iterable of byte-like objects
- Only concatenating bytes for debug logging when enabled
- Directly passing the iterable to transport.writelines, avoiding unnecessary concatenation in the data path
This implementation aligns perfectly with the PR objective to leverage Python 3.12's transport.writelines for improved performance.
Note: The performance benefit will be most noticeable when sending multiple large buffers, as it avoids the memory allocation and copying overhead of concatenating them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
aioesphomeapi/_frame_helper/base.pxd(1 hunks)aioesphomeapi/_frame_helper/base.py(5 hunks)aioesphomeapi/_frame_helper/noise.py(2 hunks)aioesphomeapi/_frame_helper/plain_text.py(1 hunks)tests/common.py(1 hunks)tests/test__frame_helper.py(9 hunks)tests/test_client.py(6 hunks)tests/test_connection.py(12 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
aioesphomeapi/_frame_helper/base.pxd (1)
Pattern **: - Do not generate or add any sequence diagrams
aioesphomeapi/_frame_helper/base.py (1)
Pattern **: - Do not generate or add any sequence diagrams
aioesphomeapi/_frame_helper/noise.py (1)
Pattern **: - Do not generate or add any sequence diagrams
aioesphomeapi/_frame_helper/plain_text.py (1)
Pattern **: - Do not generate or add any sequence diagrams
tests/common.py (1)
Pattern **: - Do not generate or add any sequence diagrams
tests/test__frame_helper.py (1)
Pattern **: - Do not generate or add any sequence diagrams
tests/test_client.py (1)
Pattern **: - Do not generate or add any sequence diagrams
tests/test_connection.py (1)
Pattern **: - Do not generate or add any sequence diagrams
🔇 Additional comments (28)
aioesphomeapi/_frame_helper/plain_text.py (2)
60-61: LGTM! Good optimization to skip empty data.
The conditional check prevents adding empty data to the output buffer, which is more efficient.
63-63: Verify transport.writelines implementation.
Since this PR aims to leverage Python 3.12's transport.writelines with IOV_MAX, let's verify the base implementation.
✅ Verification successful
Let's gather more information about the implementation details.
Let's check one more thing to understand how the _writelines is set and used across the codebase.
Transport writelines implementation is correctly used
The verification shows that the base implementation properly handles transport.writelines:
- The base class correctly initializes
_writelinesinconnection_madeby assigning it from transport - Type checking ensures
_writelinesis available before use - The implementation properly accepts an
Iterable[bytes]as required for transport.writelines
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if base class implements writelines correctly
# Expected: Find implementation of _write_bytes in base class that uses transport.writelines
# Look for base class implementation
ast-grep --pattern 'def _write_bytes(self, $_, $_)'
# Look for transport.writelines usage
rg -l 'transport.*writelines'
Length of output: 195
Script:
#!/bin/bash
# Check the implementation of _write_bytes in base.py and its usage of transport.writelines
rg -A 10 "_write_bytes" aioesphomeapi/_frame_helper/base.py
# Check test cases to understand the expected behavior
rg -A 5 "transport.*writelines" tests/test__frame_helper.py
Length of output: 761
Script:
#!/bin/bash
# Check how _writelines is initialized and used
rg -A 5 "_writelines" aioesphomeapi/_frame_helper/base.py
# Check the connection_made implementation that sets up transport
rg -A 10 "connection_made" aioesphomeapi/_frame_helper/base.py
Length of output: 1367
tests/common.py (2)
79-80: LGTM! Good backward compatibility implementation.
The function maintains compatibility with code expecting single bytes while leveraging the new split packet generation internally.
68-76: LGTM! The packet structure is correctly split for zerocopy writes.
The function efficiently splits the packet into separate byte segments that align with the sendmsg IOV structure shown in the PR comments. Using _cached_varuint_to_bytes for length and type encoding maintains good performance.
Let's verify the packet structure matches the protocol:
✅ Verification successful
Based on the comprehensive verification results, I can now provide a final response:
The packet structure implementation is correct and consistent with the protocol.
The verification confirms that:
- The packet structure in
generate_split_plaintext_packetmatches the protocol implementation inAPIPlaintextFrameHelper.write_packetsanddata_received - The packet components are correctly ordered:
- Null byte marker (b"\0")
- Length as varuint
- Message type as varuint
- Serialized bytes
- The implementation is validated by extensive test cases in
test__frame_helper.pythat verify both packet generation and parsing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the packet structure matches the protocol implementation
# Test: Check if the packet structure in other files matches our implementation
rg -A 5 "data_received.*\x00.*varuint.*bytes_"
Length of output: 48
Script:
#!/bin/bash
# Let's search for protocol implementation files and packet handling
rg -l "class APIConnection|class APIServer" --type py
Length of output: 105
Script:
#!/bin/bash
# Search for packet handling and message type mapping
rg "PROTO_TO_MESSAGE_TYPE|MESSAGE_TYPE_TO_PROTO" --type py -A 3
Length of output: 2466
Script:
#!/bin/bash
# Look for packet parsing/receiving logic
rg "async def data_received|def data_received" --type py -A 10
Length of output: 1902
Script:
#!/bin/bash
# Check test files that verify packet structure
fd "test.*packet.*\.py$"
Length of output: 26
Script:
#!/bin/bash
# Let's examine the packet structure in the frame helper implementation
cat aioesphomeapi/_frame_helper/plain_text.py
Length of output: 3579
Script:
#!/bin/bash
# Check the actual packet generation in connection.py
rg "def _message_to_json|def _format_message_for_log" aioesphomeapi/connection.py -A 10
Length of output: 89
Script:
#!/bin/bash
# Look for any test cases that verify packet structure
rg "test.*packet" tests/ -l
Length of output: 57
Script:
#!/bin/bash
# Let's check the frame helper test cases
cat tests/test__frame_helper.py
Length of output: 24959
Script:
#!/bin/bash
# Check the actual packet writing implementation in connection
rg "write_packet|write_packets" aioesphomeapi/ -A 5
Length of output: 2568
aioesphomeapi/_frame_helper/base.py (5)
5-5: LGTM: Import addition is appropriate.
The addition of Iterable import from collections.abc is necessary for type hinting the new writelines parameter.
35-35: LGTM: Slot name change reflects new functionality.
The renaming from _writer to _writelines in __slots__ maintains consistency with the new zerocopy writes implementation.
55-57: LGTM: Type annotation accurately reflects transport.writelines signature.
The type annotation properly defines the expected callable that accepts an iterable of byte-like objects, aligning with Python's transport.writelines specification.
152-152: LGTM: Connection lifecycle properly manages writelines.
The changes consistently handle the writelines assignment during connection establishment and cleanup during connection closure.
Also applies to: 178-178
152-152: Verify Python version requirement.
Since this implementation relies on Python 3.12's transport.writelines feature, ensure that the package's Python version requirement is properly updated in setup.py/pyproject.toml.
aioesphomeapi/_frame_helper/noise.py (2)
221-221: Optimization: Efficient handshake frame transmission
The change from byte concatenation to passing a list of byte sequences leverages Python 3.12's zerocopy writes capability, eliminating unnecessary memory allocation and copy operations during the handshake phase.
349-349: Optimization: Efficient packet transmission
The change leverages Python 3.12's zerocopy writes capability by passing the list of packets directly to _write_bytes, avoiding unnecessary concatenation of potentially large encrypted packets. This is particularly beneficial when sending multiple packets in sequence.
tests/test__frame_helper.py (3)
5-5: LGTM: Import of Iterable for type hints
The addition of Iterable from collections.abc aligns with the changes to support multiple buffers in writelines.
136-136: LGTM: Updated MockAPINoiseFrameHelper to use writelines
The changes correctly mock the transport to use writelines instead of write, which aligns with the PR's objective to leverage Python 3.12's zerocopy writes capability.
Also applies to: 151-151
Line range hint 441-563: Verify the test coverage for writelines error handling
The tests should verify that writelines properly handles transport errors (e.g., ConnectionResetError, OSError) and propagates them correctly.
tests/test_connection.py (9)
118-118: LGTM: Assertion correctly updated for writelines.
The assertion correctly verifies that the disconnect request is sent as multiple buffers using writelines.
155-155: LGTM: Consistent assertion pattern.
The assertion follows the same pattern as other disconnect tests, correctly verifying the writelines call.
509-509: LGTM: Protocol transport assignment.
The assignment of _writelines to the transport's writelines method is correct for the mock protocol setup.
552-554: LGTM: Test structure update.
The test case structure is properly updated with correct indentation and context management.
651-651: LGTM: Error simulation.
The mock correctly simulates an OSError during writelines to test error handling during disconnect.
898-898: LGTM: Ping test assertions.
The ping test correctly:
- Defines the expected ping request bytes
- Verifies the call count
- Validates the exact calls made
- Checks the final state
Also applies to: 909-909, 911-911, 920-920
937-937: LGTM: Ping response handling.
The test properly verifies that only one ping request is sent when responses are received.
Also applies to: 950-951
981-983: LGTM: Ping request response.
The test correctly verifies the response to a ping request using the new writelines interface.
Line range hint 898-983: Consider adding buffer size verification.
While the tests verify the correct sequence of bytes being sent, they could be enhanced to verify that each buffer in writelines is under the system's IOV_MAX limit.
tests/test_client.py (5)
129-129: LGTM: Import added for new packet generation utility
The addition of generate_split_plaintext_packet import aligns with the PR's objective to implement zerocopy writes by supporting split packet generation.
1443-1448: LGTM: Test assertion verifies correct packet structure
The test assertion correctly verifies that the transport writelines is called with the expected packet structure for bluetooth GATT write operations. The packet is split into multiple buffers:
- Control byte
- Length byte
- Message type
- Payload
2056-2057: LGTM: Test assertion verifies zerocopy write behavior
The assertion correctly verifies that the transport writelines is called with the split packet generated by generate_split_plaintext_packet for bluetooth device requests.
2147-2147: LGTM: Test verifies transport writelines call count
The test properly verifies the sequence of transport writelines calls:
- Initial connect request
- Subsequent disconnect request after timeout
Also applies to: 2153-2153
2191-2191: LGTM: Test verifies initial connect request
The assertion correctly verifies that the initial connect request is written via transport writelines.
With Python 3.12+ and later
transport.writelinesis implemented assendmsg(..., IOV_MAX)which allows us to avoid joining the bytes and sending them in one go.Older Python will effectively do the same thing we do now
b"".join(...)