Use faster-fifo for queues & more performance improvements
#99
Conversation
Co-authored-by: Harshil <[email protected]>
|
Tested this on the Pi extensively today. It works very well with no practical downsides. We no longer get any "spikes" in fetched packets. Actual queue size is also <2 when run in real time. |
|
@JacksonElia how badly do you want Windows support? I think it's trivial to keep windows support but it might hurt readability just a little bit |
|
probably at the top of the files that use the queues we have an if statement that checks which os you're on, and depending on that imports the queue that we want with a common alias |
Yes that's what I was thinking. So you do want Windows support? |
|
Yeah, when doing development it is so important/crucial to be able to run the mock |
|
Alright just added Windows support. @JacksonElia can you confirm if it works? Need to test this one last time on the pi before merging. |
Provides about ~60% speedup, from 8.5us to 5.4us
Improves performance by about ~9%, 142k ns -> 129k ns
Improves performance by about ~2-5%
Default dtype is np.float64 anyway. About ~20-25% speedup
… deque It's an improvement because we don't have to convert the deque to a list when sending it to the apogee prediction process. Saves about 0.14s (100k iterations)
harshil21
changed the title
faster-fifo for queuesfaster-fifo for queues & more performance improvements
|
Latest commit adds a couple of tests which I believe are robust enough that I don't need to test this PR on the Pi before merging. If one of you can confirm that this works on Windows within 24 hours, I can merge (still need an approving review). Otherwise, I have a Windows machine nearby I can quickly test on. |
|
Alright I was able to test and fix the Windows runtime myself. Also added the integration test for Windows here in the CI. Getting the unit tests to work on Windows is not worth it imo. Windows is going to be a 2nd class citizen as long as we're using |
There was a problem hiding this comment.
Looked over everything. Code runs on my system, and you said you tested on the Pi. Tests look good too. I added that one comment for the imu test but seems fine to merge. Maybe an issue for another time, but I feel like it might be less repetitive to handle a lot of the if/else sys.platform checks in a utils function that can create all of the multiprocessing queues and related things itself. Not a big concern for the moment though, this is already a lot lmao
|
oh yeah I guess that unit test needs to be fixed. idk why thats failing though |
There would've been an exception in the apogee prediction and logger process after 100 seconds of just sitting on the pad. This was an oversight from #99. Tests have been added to detect this.
Switches to the faster-fifo library to bring about massive performance improvements to the mock sim and to the pi, even in real time.
Also optimizes logger buffer logic a little bit and a bunch of small things to improve performance.
Part of #85.