Refactor ReadConsole P/Invoke in ConsoleHost#9165
Refactor ReadConsole P/Invoke in ConsoleHost#9165daxian-dbw merged 7 commits intoPowerShell:masterfrom
Conversation
|
Windows uses 8 Kb buffer to read from console If we could use a buffer of the same size on Windows, we could use a stackalloc instead of an ArrayPool. Performance ArrayPool vs allocation https://adamsitnik.com/Array-Pool/ |
| lastCompletion = completedInput; | ||
| } | ||
|
|
||
| lastInput = completedInput; |
There was a problem hiding this comment.
Does tab completion still work when PSReadLine isn't loaded?
There was a problem hiding this comment.
Yes, it does not work. I will look more closely.
I see and wonder that tab completion doesn't work on Unix without PSReadline at all.
There was a problem hiding this comment.
@iSazonov Unix has a very simple readline implementation while Windows still enables the tab-complete experience (using Windows APIs for rendering). Want to make sure we don't introduce a regression here.
There was a problem hiding this comment.
On Windows, the cooked input mode supports ending on Tab to support tab completion in a cli like PowerShell or cmd, see here.
On Unix, cooked mode does not end on Tab and a cli typically ends up in raw mode if it wants tab completion.
|
@iSazonov - the As for buffer Windows/Unix buffer sizes - I believe the code you're looking at only matters if |
|
@lzybkr Thanks! Although you have approved this change, I still have to restore tab completion on Windows, yes? |
|
Oh, I probably didn't look closely enough at this, you shouldn't regress tab completion w/o PSReadLine. I was thinking the |
|
The feature request to enhance Console.ReadKey() to complete at Tab. Update: already implemented! |
bf12348 to
dd0514d
Compare
dd0514d to
4bbf15d
Compare
|
I pushed new commit to revert previous changes and use stack edit buffer (size reduced from 8Kb to 1Kb). |
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostUserInterface.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleControl.cs
Outdated
Show resolved
Hide resolved
@lzybkr After some investigations I discovered that canonical (cooked) input mode on Unix may ends line input by VEOL which can be set to TAB by termios API If this is the right direction I could open new tracking Issue. Maybe there is still a problem getting the cursor position. |
|
There's little harm in opening an issue, but I'd have a hard time justifying spending much time on that. Windows has the support because that code was written before PSReadLine existed. We opted to more or less require PSReadLine on non-Windows because it wasn't hard to port and had all of the key functionality folks expect on non-Windows. |
|
@lzybkr Thanks for your comment! I also discover that we call TryInvokeUserDefinedReadLine() for every new input line where we do search |
|
It’s probably not worth the trouble. The biggest performance problem was searching for external commands and that is no longer an issue, the search skips those now. I had a similar idea when we included PSReadLine in Windows and actually implemented my idea to call the C# api directly. It caused problems with custom scriptblock handlers. I decided that in this case it was best to keep the code simple. |
| } | ||
|
|
||
| private const int maxInputLineLength = 8192; | ||
| private const int MaxInputLineLength = 1024; |
PR Summary
Replace
StringBuilderwith stack allocatedSpan<char>to avoid extra allocation/copies during P/Invoke.PR Context
Using StringBuilder in P/Invoke causes extra allocations and buffer copies.
Related #9139
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.[feature]to your commit messages if the change is significant or affects feature tests