Migrate from synchronous to async read #127
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To improve the performance. The current implementation severely underperforms, reaching at maximum 60 Mbit/s on a local 1 Gbit/s link (where the cmd tool `scp` performs at over 800 Mbit/s). With video bit rates reaching 60 Mbit/s, videos can no longer be streamed without occasional interruptions. This problem severely worsens if the network latency is higher. One situation with about 40 ms RTT latency only reached about 10 Mbit/s, even though the network could handle more throughput. See xbmc#22 for more discussion. This work always initiates the next read at the end of each previous one. This however requires to specify a length, which might not match with the length Kodi requests next (e.g. at the end of file). To avoid memory leaks, we have to call `sftp_async_read` with a valid buffer of the correct size. This means a temporary buffer is needed. If too much was actually read, this work simply rewinds the current file offset to the Kodi-expected place. This might also mess with other/threaded usage, as the async work can run concurrently with normally mutexed areas, and even is expected to change the file position during retrieval, leading to inconsistent results when calling `GetPosition` at the same time.
|
This PR sounds interesting, as I'm also facing those issues with SFTP VFS addon when accessing the server with high RTT, even the connection throughput is enough. May I ask: Are you able to provide ARM64 build with this patch applied? I would like to test that on my Android box. |
Sorry, no. I test this directly with NixOS on my machines. Additionally, I should note that a file copy in Kodi's file manager seemed to work fine, but playback of a movie failed to even start playing. So this code is not really working rn. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To improve the performance. The current implementation severely underperforms, reaching at maximum 60 Mbit/s on a local 1 Gbit/s link (where the cmd tool
scpperforms at over 800 Mbit/s). With video bit rates reaching 60 Mbit/s, videos can no longer be streamed without occasional interruptions.This problem severely worsens if the network latency is higher. One situation with about 40 ms RTT latency only reached about 10 Mbit/s, even though the network could handle more throughput. See #22 for more discussion.
This work increases the throughput on the same setup from 60 Mbit/s to almost 100 Mbit/s.
This work always initiates the next read at the end of each previous one. This however requires to specify a length, which might not match with the length Kodi requests next (e.g. at the end of file). To avoid memory leaks, we have to call
sftp_async_readwith a valid buffer of the correct size. This means a temporary buffer is needed. If too much was actually read, this work simply rewinds the current file offset to the Kodi-expected place.This might also mess with other/threaded usage, as the async work can run concurrently with normally mutexed areas, and even is expected to change the file position during retrieval, leading to inconsistent results when calling
GetPositionat the same time.This PR "works" but I don't feel confident in error conditions, thread safety and others.