Console: Unix: Improve Cursor{Left,Top} performance by caching them #36049
Conversation
We keep a cached version for CursorLeft and CursorTop. When we write data to stdout, we try to update the cached values based on the buffer content, or we invalidate the cursor position. A lock is used to deal with multi-threading issues. We synchronize between some blocks using a 'version' field. This field is incremented each time a cursor change may have occured. If the value has not changed between two blocks, we know the the cursor is still at the same position. On signals that may mean the cursor position has changed (SIGCONT, SIGCHLD, SIGWINCH) we invalidate the cached values.
|
@stephentoub PTAL. It was a challenge to get to this PR. |
…o need to invalidate after writing
|
@tmds did you get a chance to do any perf testing before/after this change? Would be interesting to see what kind of improvements we're getting. Thanks a lot for doing this! |
I took the code snippit that was included in https://github.com/dotnet/corefx/issues/32174. Pre PR: Post PR: |
| @@ -109,7 +126,10 @@ public static ConsoleKeyInfo ReadKey(bool intercept) | |||
| keyInfo = new ConsoleKeyInfo('\r', keyInfo.Key, shift, alt, control); | |||
| } | |||
|
|
|||
| if (!intercept && !previouslyProcessed) Console.Write(keyInfo.KeyChar); | |||
| if (!intercept && !previouslyProcessed && keyInfo.KeyChar != '\0') | |||
There was a problem hiding this comment.
@stephentoub @wtgodbe I've added != '\0 check here. This isn't related to the cursor caching. The StdInReader.ReadLine method also doesn't write these, and I think it is safe to filter those out here too.
There was a problem hiding this comment.
I hadn't noticed the question. It stops from doing a single byte write with '\0' that is ignored by the terminal.
There was a problem hiding this comment.
Sure, thanks. But what causes the null in the first place? Is it actually common?
There was a problem hiding this comment.
For example, if you press an arrow key, you get a '\0' char.
|
Thanks! |
|
Looks like this change is causing a CI failure in new (unrelated) PRs (e.g. #36229):
Where is this constant defined? |
|
I'll fix it. Two PRs crossed. |
…otnet/corefx#36049) * Console: Unix: Improve Cursor{Left,Top} performance by caching them We keep a cached version for CursorLeft and CursorTop. When we write data to stdout, we try to update the cached values based on the buffer content, or we invalidate the cursor position. A lock is used to deal with multi-threading issues. We synchronize between some blocks using a 'version' field. This field is incremented each time a cursor change may have occured. If the value has not changed between two blocks, we know the the cursor is still at the same position. On signals that may mean the cursor position has changed (SIGCONT, SIGCHLD, SIGWINCH) we invalidate the cached values. * PR feedback * lock on Console.Out and rework invalidation on signals * fix: pass cursorVersion also in UpdateCachedCursorPosition * Since we check invalidated before reading cached settings, there is no need to invalidate after writing * Use GetWindowSize instead of getting WindowWidth, WindowHeight * Update comment * Fix swapped width and height * ReadKey: avoid write call for '\0' characters Commit migrated from dotnet/corefx@be7db25
We keep a cached version for CursorLeft and CursorTop. When we write
data to stdout, we try to update the cached values based on the buffer
content, or we invalidate the cursor position.
A lock is used to deal with multi-threading issues. We synchronize
between some blocks using a 'version' field. This field is incremented
each time a cursor change may have occured. If the value has not
changed between two blocks, we know the the cursor is still at the same
position.
On signals that may mean the cursor position has changed (SIGCONT,
SIGCHLD, SIGWINCH) we invalidate the cached values.
Fixes https://github.com/dotnet/corefx/issues/32174
Fixes https://github.com/dotnet/corefx/issues/31517
Contributes to https://github.com/dotnet/corefx/issues/34885