Optimize con 8000 sole output in ConsoleHost#6882
Conversation
|
CI Appveyor temporary failed on Update-Help tests. |
|
Reopen the PR to restart CIs. |
| fixed (char* bufferPtr = &MemoryMarshal.GetReference(buffer)) | ||
| { | ||
| return WriteConsole(consoleOutput, bufferPtr, numberOfCharsToWrite, out numberOfCharsWritten, reserved); | ||
| } |
There was a problem hiding this comment.
This obviously complexes the code. Actually, all changes here make code more complex. Do we have measurable perf improvement to justify the changes? I don't see a large amount of string manipulation here (please point it out if I miss them). Note that many string APIs and StringBuilder already received perf improvements in 2.1.
There was a problem hiding this comment.
It looks like complement - I'm not sure I can create a complex code 😄
Exactly the pattern I found in CoreFX. This is due to C# language limitations.
Really the code isn't getting any more complicated. Rather on the contrary it becomes more complete.
By analyzing the code to clean up psl I found that in this place we make a lot of kernel calls and extra locks because of the multitude of:
WriteToConsole(text, true);
WriteToConsole(Crlf, true);I was very surprised that we don't have a WriteLine() method to write to a host and use many allocations like value + Crlf - so I just implemented the method.
The only complicity of the code is that I had to modify three levels of interfaces. So really the PR is very simple.
Note that many string APIs and StringBuilder already received perf improvements in 2.1.
Yes, I saw up to 1100000 allocations in VS Profiler at pwsh.exe 6.0 startup and up to 850000 allocation at 6.1 (already on .Net 2.1) startup. Nevertheless, I still see a lot of allocations. The profile show that up to 36% allocations is chars and strings.
This shows that CoreCLR does not solve all allocation problems and we should continue to consume resources cautiously and to optimize our code.
There was a problem hiding this comment.
I'm not convinced whether the startup is the right scenario to measure for changes in this PR, but still, for the startup scenario, what's the improvement in object allocation with your changes?
There was a problem hiding this comment.
No, it was only sample that 2.1 reduce allocations but not remove its at all. I don't know how measure wins in the PR - VS Profiler is crashed after first run. I look https://benchmarkdotnet.org/ but it takes a lot of effort. At least in the first steps.
There was a problem hiding this comment.
I attached a file with simple perf tests. I saw win ~2 second on 100000 outputs.
measure_conhost.txt
There was a problem hiding this comment.
Do we really need any of this code anymore? It was written back in V1 when the Console API was very limited. I suspect that we could remove most of the code with just a thin wrapper around the .NET APIs. (Which, in fact, were somewhat inspired by this code.)
There was a problem hiding this comment.
I agree that this code would have to be cleaned more thoroughly. The problem is that we do not have tests for the console code. Therefore, I would prefer to make small changes step by step so as not to break something. I intend to continue working in this direction (if only you do not chop it in one your PR).
|
@daxian-dbw I changed PR title - seems old one confuse you that is main win in the PR. |
|
We could use https://benchmarkdotnet.org/ for perf analyses. |
|
@iSazonov I like the perf analysis idea. We definitely need it for benefits in the long run. I will look into it. |
|
@daxian-dbw Not related the PR - I passed 200 MB file through ConvertFrom-Csv and get alarming results. |
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
|
@daxian-dbw @anmenaga @SteveL-MSFT Please review the PR. |
|
@daxian-dbw Could you please continue the PR review? |
|
@iSazonov I'm busy on some investigations and 6.1-must-have issues recently (RC will be next week and need to get those in). Will resume reviewing this PR in a few days. |
|
A ping for myself so that the stale bot doesn't kick in :) |
|
ping -l 1496 |
8fa07db to
7d5c1ec
Compare
|
Rebased to update CIs. |
|
@daxian-dbw friendly ping for review. |
7d5c1ec to
3d87f96
Compare
|
@SteveL-MSFT Have MSFT team special thoughts about the PR? Can we continue or close? |
|
@iSazonov @daxian-dbw is currently on vacation and will be back next week. |
|
@daxian-dbw friendly ping for review. |
|
I think @adityapatwardhan made some changes so that now Linux/macOS CIs always run feature tests. It looks to me your changes cause some feature tests to fail. |
|
@daxian-dbw Thanks! I I looked through the CIs history - feature tests was turned on but not reported in right way then this was fixed and now we see the CIs fail. |
1a181ac to
dee0dfd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dee0dfd to
c97670d
Compare
|
I discover that input echoing to output in the failed tests but I can not find the cause. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Last commit fix the problem but I wonder and don't understand how it works :-) |
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| internal void WriteToConsole(char c, bool transcribeResult) | ||
| { | ||
| Span<char> value = stackalloc char[1] { c }; |
There was a problem hiding this comment.
As I mentioned in #6882 (comment), please use ReadonlySpan directly to avoid the unneeded conversion from Span to ReadonlySpan
| Write(char c) | ||
| { | ||
| this.Write(new String(c, 1)); | ||
| Span<char> c1 = stackalloc char[1] { c }; |
There was a problem hiding this comment.
Same here, use ReadonlySpan directly.
| { | ||
| if (newLine) | ||
| { | ||
| WriteConsole(consoleHandle, ConsoleHostUserInterface.Crlf.AsSpan()); |
There was a problem hiding this comment.
I believe you don't need the AsSpan part.
|
@iSazonov I left 3 comments, but pushed 2 commits to address them and do some tiny refactoring. The second commit's message describes itself: Please take a look to see if the changes make sense. Thanks. |
|
@daxian-dbw Sorry, I missed some fixes again. I only have short periods of time to do something useful and for forced pauses I lose context :-( |
|
@daxian-dbw Thanks for review and help! I do not remember what I planned in the continuation of this work. I will look again :-) The other day I was playing with a small application and added output to the console - this application catastrophically slowed down (up to 10 times). Now I am concerned that our intentions to replace our code with Console.* can lead to negative consequences. |
|
The plan was to replace P/Invoke calls with dotnet Console APIs.
I double it's because of the Console API. It could be the fact of writing to console itself caused this difference, no matter you were using P/Invoke or the managed Console API. |
The dotnet API (
Yes, but there is still quite a thick wrap in dotnet. |
Let's first focus on replacing the write P/Invoke. We can get back to the read operations later. I would like small PRs :)
PSReadline is refactored to move away from P/Invoke to make it more portable. I think perf impact won't be a concern unless we can prove it's noticeable. |
My current understanding that we can't even do it for Windows - It seems there is no "input completion by tab" in |
|
|
|
I can try it. Until I am sure to turn out to completely remove P/Invokes for writing. |
PR Summary
WriteLinemethod to write to a host. Win is that we exclude extra string allocations likevalue + Crlfand extra locks and kernel calls.keyInfo.Character.ToString()) inReadKey(ReadKeyOptions options).WriteLine-like methods to replaceWrite(strValue + Crlf)withWriteLine(strValue).Write(strValue); Write(Crlf)withWriteLine(strValue).WriteToConsole(ConsoleColor foregroundColor, ConsoleColor backgroundColor, string text, bool newLine = false).ReadOnlySpan.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature testsThis change is