Improve serialization/deserialization for raw bytes - #1
Conversation
julianadrianheine
changed the title
| if code == 'B': | ||
| return self.write_bytes(bytes(values)) | ||
| a = _array.array(code, values) | ||
| if (self.endianness == Endianness.Little) != (sys.byteorder == 'little'): |
There was a problem hiding this comment.
Seems this check is redundant, right?
See the class definition:
self.set_endianness(Endianness.native())
There was a problem hiding this comment.
It looks like even when the endianness is being initialized in the constructor, there are parts of the code that can set it again, based on the CDR encapsulation header in _main.py:
if has_header and buffer.tell() == 0:
buffer.read('b', 1)
v = buffer.read('b', 1)
if (v & 1) > 0:
buffer.set_endianness(Endianness.Little)
else:
buffer.set_endianness(Endianness.Big)
| @@ -10,6 +10,7 @@ | |||
| * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause | |||
There was a problem hiding this comment.
Meta: Some tests are supposed to be run. Can we verify that all tests still pass CI? And add tests if needed? Should we set up a github action to run these tests?
There was a problem hiding this comment.
Sure, let me take a look at it
imcmahon-tri
left a comment
imcmahon-tri
left a comment
There was a problem hiding this comment.
Thanks for these changes! The actual code changes look great. A few points:
- Consider separating the wheel changes separated into their own PR.
- It would be useful to have docstrings or comments describing the necessity for the new functions.
- Adding some unit tests which check functionality but also show a performance baseline improvement would be appreciated.
| @@ -0,0 +1,78 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
For clarity, could you carve out the wheel building into it's own PR? That will make it easier to refer to this PR when discussing the serialize & deserialize improvements with upstream.
| self._pos += size | ||
| return v | ||
|
|
||
| def write_sequence(self, code: str, values) -> 'Buffer': |
There was a problem hiding this comment.
The (de)serialization improvements look good. Could you add comments or docstrings to describe why these functions are necessary and perhaps how they address the byte/list issue?
| a.byteswap() | ||
| return self.write_bytes(a.tobytes()) | ||
|
|
||
| def read_sequence(self, code: str, length: int) -> list: |
There was a problem hiding this comment.
Have you considered adding unit tests for these new functions? This PR is making performance improvement assertions but lacks a concrete metric showing how it improves the situation. Consider adding a metric to show the read/write performance with & without the write/read_sequence functions.
There was a problem hiding this comment.
Done! I added some tests, as well as some metrics in the PR description
There was a problem hiding this comment.
Thanks for adding these comments and tests.
4cf3954 to
c6f4cd2
Compare
imcmahon-tri
left a comment
imcmahon-tri
left a comment
There was a problem hiding this comment.
:LGTM: / 
Thanks for iterating on this. It looks great and is a very helpful explanation of the issue you solved.
| a.byteswap() | ||
| return self.write_bytes(a.tobytes()) | ||
|
|
||
| def read_sequence(self, code: str, length: int) -> list: |
There was a problem hiding this comment.
Thanks for adding these comments and tests.
3 participants
Description
Improve the serialization and deserialization when handling raw bytes in
cyclonedds-pythonContext
When writing sequences of bytes, the CDR serializer was iterating through the data element by element using a generic slow path. We added a fast path that detects when the sequence is already a bytes object and writes it directly as a raw buffer, avoiding the per-element overhead.
What is being modified
cyclonedds/idl/_support.py: Two new methods added toBuffer(write_sequenceandread_sequence) that use Python's built-inarraymodule to serialize and deserialize homogeneous primitive sequences in a single bulk copy, followed by a one-shotbyteswap()when wire and host endianness differ.uint8is special-cased tobytes()for an additional speedup.cyclonedds/idl/_machinery.py:PlainCdrV2ArrayOfPrimitiveMachineandPlainCdrV2SequenceOfPrimitiveMachinenow delegate to the new bulk methods instead of building a length-prefixedstructformat string on every call and unpacking elements one by one.Results
The benchmark covers two sequence sizes: N=100 (small sequences, representative of small payloads) and N=100000 (large sequences, representative of high-throughput payloads). All five primitive types supported by the CDR machinery are included. Times are in µs, averaged over 300 iterations.
The largest gains appear on
uint8at large N (up to -80% ser / -70% des) due to thebytes()fast path. Multi-byte types show consistent 10–36% improvement at large N. At small N the gains are more modest since fixed per-call overhead dominates over the bulk copy savings.PlainCdrV2ArrayOfPrimitiveMachine
PlainCdrV2SequenceOfPrimitiveMachine