Improve responsiveness of conhost/ConPTY for large inputs #11890
Conversation
ghost
added
Area-Performance
| // But I'm not too concerned that this will lead to issues at the time of writing, | ||
| // as CursorBlinker is allocated as a static variable through the Globals class. | ||
| // It'd be nice to fix this, but realistically it'll likely not lead to issues. | ||
| gci.LockConsole(); |
There was a problem hiding this comment.
There's no TryLockConsole() anymore, so I replaced it with a worse solution. 😅
Using the new thread pool API we can simply tell the OS to not block us on exit. It's simpler so that's a win at least.
There was a problem hiding this comment.
This was really the only reason that TryLockConsole existed in the first place? Wow.
| static thread_local ULONG recursionCount = 0; | ||
| static til::ticket_lock lock; |
There was a problem hiding this comment.
Ah btw: You can't have thread locals as class members. They only work as statics / in the global scope.
src/host/CursorBlinker.cpp
Outdated
| LOG_LAST_ERROR_IF(!bRet); | ||
| } | ||
| const auto periodInMS = _uCaretBlinkTime == -1 ? dwDefTimeout : _uCaretBlinkTime; | ||
| // The FILETIME struct measures time in 100ns steps. 1ms is 10000ns. |
There was a problem hiding this comment.
You mean 10000 * 100ns = 1ms, right?
There was a problem hiding this comment.
Yeah exactly. I'll correct it to say "10000 steps" instead.
| // But I'm not too concerned that this will lead to issues at the time of writing, | ||
| // as CursorBlinker is allocated as a static variable through the Globals class. | ||
| // It'd be nice to fix this, but realistically it'll likely not lead to issues. | ||
| gci.LockConsole(); |
There was a problem hiding this comment.
This was really the only reason that TryLockConsole existed in the first place? Wow.
ghost
requested review from
zadjii-msft,
carlos-zamora,
leonMSFT and
PankajBhojwani
|
Admin merge - this predates the splitting out of the "Test" phase |
|
🎉 Handy links: |
|
This PR is unrelated to your issue as far as I know. I believe the input slowness is caused by the poor architecture inside conhost. If you ever want to improve it, you're absolutely welcome to submit PRs that improve our performance. It's probably easiest to find such performance issues using WPR (Windows Performance Recorder) and WPA (Windows Performance Analyzer). I'm also working on improving its performance but I'm just one person and conhost is ~500k LOC, many parts of which need similar improvements. |
This commit replaces the console lock was replaced with a fair ticket
lock implementation, as only fair locks guarantee us that the renderer
(or other parts) can acquire the lock, once the VT parser has finally
released it.
This is related to #11794.
Validation Steps Performed