Significantly improve performance of ShellStream's Expect methods

[jscarle]

I'm glad to see that the project is alive again! This is an updated PR based off the original PR #793 as the original issue still exists and it is a very serious performance issue.

Expect's performance degrades quickly as the size of the _incoming Queue grows. The amount of work that needs to be done by Regex grows with each byte added. Using a Regex pattern to detect a Bash prompt, I ran into an issue while Expecting the prompt whilst doing a large yum update on a Linux server. The resulting queue size jumped into the megabyte region and running a Regex match against the _incoming queue brought the process to a crawl. What would normally take about 3 minutes on a bash shell, was taking hours. After more than 2 hours, I cancelled the process and started debugging the issue with JetBrains' dotTrace. 85% of the process execution time was spent on the Regex Match.

I added a parallel _expect Queue and a _expectSize parameter to the ShellStream to allow a synchronous buffer to run along side of the _incoming queue, but with a limited capacity equivalent to _expectSize. As a default overload for CreateShellStream, if the parameter is omitted, it uses the number of columns as a default _expectSize. This allows for a running windows for Regex to check its Expect pattern and that windows remains small independent of the actual size of the _incoming queue. In my tests, this completely eliminated the slow down caused by the ever increasing size of the _incoming queue. It allows for performance on par with being directly connected as a human against the bash shell.

Whilst working on this, I noticed an additional issue that was simple to resolve given the now available parallel _expect queue.

Considering the default Encoding is UTF-8, there Regex Match Index does not necessarily correspond to the actual byte position within the UTF-8 string as some characters can be double byte encoded, which affects the Index returned by Match. ASCII does not support double byte encoding, so for the purpose of Expect, it makes more sense to match against an ASCII encoding of the string instead of a UTF-8 encoding since the _incoming Queue is obviously encoding agnostic.

Running a seperate Expect queue allows to Match against an ASCII version for byte position fidelity whilst conserving the proper encoding when returning the string from Expect.

Here is a dotNetFiddle that demonstrates the Match Index position issue with UTF-8 encoding (pulled from real-world result that I debugged and encoded as a byte array for dotNetFiddle): https://dotnetfiddle.net/JM80ea

[drieseng]

@jscarle, I haven't had time to go through you changes, but can you create a separate PR for the bug fix? Make sure to add a unit or integration test for it too.

[jscarle]

@drieseng This is the "minimum" change needed to fix the issue. It requires running a parallel "expect queue" to lower the memory overhead of the shell stream.

[WojciechNagorski]

Thanks for reporting this problem, because the problem exists.

I've checked this PR and I think there is a simple way to improve this.

[jscarle]

No changes have been brought to this PR as I still believe that the only way to solve the performance issue with Expect without changing the API signature of the methods is to introduce a rolling buffer that runs in parallel with the incoming queue and to use that rolling buffer as the source of the expect verification.

[jscarle]

@WojciechNagorski Can you review my comments please?

[WojciechNagorski]

Yes I can. I know, you are right there is the huge performance problem. However, I need to find more time to delve deeper into this.

[jscarle]

@WojciechNagorski The changes I've proposed can be merged as is since they both a) fix the performance issue and b) do not change any of the public APIs. This would allow everyone to benefit from the huge performance gain this brings and at a later time, if you find a better approach when you have more time, then it could be refactored. At least, this would immediately solve a huge pain point.

[WojciechNagorski]

I'm not sure. I need to find time but you know I'm doing it in my free time.

[Rob-Hague]

Sorry @jscarle that this is arduous. I think it's partly explained by ShellStream having many preexisting problems (inefficient being just one) and little (useful) test coverage. That creates a high bar for the motivation to touch it (that's been my feeling anyway), and especially to increase the complexity and potentially change the behaviour without the testing situation changing.

I've opened #1313 to add some (failing) tests. I'll try and fix them separately, and then we can revisit this improvement with more confidence?

[jscarle]

I deeply understand the challenges faced by this being an open source project to which you both volunteer your free time, and that this is a complex portion of the code to which everyone is hesitant to modify.

However, I would like to reiterate that the issue with Expect is so deep that this makes its completely unusable in any long running scenario as the time to process the buffer raises exponentially with its length.

In my real world usage, I used ShellStream to update Linux virtual machines. When the base image was fairly recent, running a yum -y update would take 5 minutes. When the image was a few months old, it would take up to an hour. After a year, the update would take up to 2 hours. I would later find out that running the same deployment steps, which I had automated, by hand would only take 20 minutes. That 1:40 difference was caused by Expect. That I how I found this issue. The code I submitted in this PR has been running in production for several years now, which is why I have such confidence in this fix.

The changes made in this PR were done with great care to solve only this issue, and nothing else. No refactoring or other improvements were done. They were also done in a way as to not do any changes to any of the public APIs, thus no breaking changes.

Internally all that is really happening is that a parallel buffer is run that is used only for the Expect. This buffer is fixed in size and rolls over to keep it concise. This eliminates all peformance issues of Expect whilst not changing any of the current behavior of ShellStream.

[Rob-Hague]

Understood. I can definitely believe ShellStream is the bottleneck in a process. And I agree with this direction in order to avoid exponential regex matching. It's good to know you've been running this for a while in production.

[WojciechNagorski]

This PR does not compile after merge with master.

[jscarle]

Build has been fixed and all tests are passing.

[WojciechNagorski]

I'm going to add more tests.