worker: use pickle instead of pyon for ipc #1675
Conversation
charlesbaynham
force-pushed
the
worker_comms_speed
branch
from
feee0af to
352183e
Compare
Pickle was slower than pyon when I checked (many years ago), I guess this has improved now... |
|
I think the more recent python stdlib include c binding by default, whereas you used to have to opt-in. That's a good point though: I ran these tests on python 3.9, I should try them on 3.6 as well. |
artiq/master/worker.py
Outdated
|
|
||
| # Write the size of the data, delimited by \n. Then, write the data | ||
| # itself. This might contain \n chars, but the receiving reader knows | ||
| # how many bytes it's expecting so it doesn't need delimiters |
There was a problem hiding this comment.
Why not move everything to a binary protocol with length (with struct.pack)+binary data and no special meaning for \n?
There was a problem hiding this comment.
Could do, but the length needs delimiting in some way and newline delimiting is already implemented and well tested.
There was a problem hiding this comment.
The loading of the binary data does completely ignore \n, it's only relevant for the length data.
There was a problem hiding this comment.
the length needs delimiting in some way
Just make it a 32-bit integer (fixed four bytes). Are you planning to send messages larger than 4GB?
There was a problem hiding this comment.
True. For Dask, again, it's not so simple: messages can come in arbitrary numbers of frames, so I've tweaked the header to be "<size of header>,<size of chunk1>,<size of chunk2>" etc. It probably could be made fully binary if we wanted, but it's not at all performance-critical and this is a pretty easy way of doing it.
charlesbaynham
force-pushed
the
worker_comms_speed
branch
from
352183e to
dcdea9f
Compare
charlesbaynham
force-pushed
the
worker_comms_speed
branch
from
dcdea9f to
9cf0ccc
Compare
|
|
instead of pickle, why won't we even consider msgpack, cbor or even json? these are stable format and we could use them to leverage serde in our firmware. |
|
@stevefan1999 please read the prior discussion ("see #1674 for details") before commenting. There are also no plans to use serde in the firmware for performance reasons, and the protocol is simple enough not to require serialization libs anyway. |
…nging based on what is / isn't installed
|
@charlesbaynham These files will need an update: |
| """ | ||
| Record known compressors | ||
|
|
||
| Includes utilities for determining whether or not to compress |
There was a problem hiding this comment.
Sounds weird to compress data for inter-process communication. IPC is not supposed to be slow and compressing data to work around IPC slowness sounds like a band-aid.
ARTIQ Pull Request
Description of Changes
Change the ipc serialization format for worker comms to pickle instead of pyon.
This PR is not complete: see #1674 for details.
Related Issue
Type of Changes
All Pull Requests
git commit --signoff, see copyright).Code Changes
flake8to check code style (follow PEP-8 style).flake8has issues with parsing Migen/gateware code, ignore as necessary.Documentation Changes
cd doc/manual/; make html) to ensure no errors.Git Logistics
git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.git show). Format:Licensing
See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.