Open
Conversation
This file contains hidden or 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
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.
Low allocation SshCommand
Fixes #1776 and #1608
(includes #1777)
Problem
SshCommand currently allocates a lot of data and doesn't allow directly processing it. This is causing memory issues and GC pressure on low memory devices or when issuing many commands at the same time. A low allocation version should be essential in my opinion for a library optimized for parallelism.
Allocations
Example:
Allocated 298.272 bytes while the output might not even be needed. the thing to note here is that this is more than the data in itself, the utf8 data is only 70.000 bytes, somehow it is allocating this at least 4 times.
Allocations i could find:
70kB *4 = 280 kB which is around what we get.
Even when clearing the pipebuffer, since a stream is not event driven it will with sudden bursts of data still enlarge the buffers and cause large allocations.
Additionally, small commands that are not even expected to give output will always allocate a minimum of 4kB of which 2kB of pipestream buffer.
Example:
This will allocate 400kB of buffers, of which 200kB might be completely wasted if the output is never used and then still needs to get collected by the GC.
Event vs Stream
While i respect the stream approach which makes it compatible with a lot of dotnet build in systems currently we are doing:
This also causes other issues like #1608 where continuity is lost. The events could simply be exposed and never need to be handled.
Fire and forget
Right now the buffers are always used and filled, if you don't read them they keep growing. For long running fire and forget processes that means you deliberately need to set up 2 threads to spin and read the buffers or they will keep growing indefinitely, leading to more wasted memory (16kB stream copy buffers) and wasted cpu:
So right now
var c = client.RunCommand("...");just cannot be used for long running processes.The minimum for fire and forget is:
Proposal
Breaking changes
The first method would be exposing the events in SshCommand and only allocating the buffers when used, but this would be a breaking change. Since that is to be avoided i believe this is not a good solution.
Allocation free SshCommand alternative
This is included in this pull request in the form of "SshCommandLite". It exposes the events through a user friendly eventarg allowing both directly using the bytes as well as the text.
Allocations solved
No streams are used and both the 2kB minimum and the ever growing buffer are solved. Output can still be obtained by using the events
client.CreateCommandLite("...");only allocates 184 for SshCommandLite and the rest is Session bufferclient.RunCommandLite("for i in {1..1000} ; do echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ; done")only allocates 5kB in session buffer, nothing elsewhereEvent vs Stream
Since events are accessible, this solves continuity issues and improves efficiency due to no extra wasted cycles on buffer copies.
Fire and forget
Fire and forget is now actually possible since no output keeps growing while its not captured.
I agree this is a rather "large" change you might not be willing to do, but i decided since otherwise the library just isn't usable for me i need to make my own fork either way if it is not accepted. No hard feelings if you discard it. But please do take a look at the problems shown here as they might be bothering other people as well.
Thank you and sorry for the long read.