-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve responsiveness of conhost/ConPTY for large inputs #11890
Conversation
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah btw: You can't have thread locals as class members. They only work as statics / in the global scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this. Thanks for figuring it out, Leonard.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean 10000 * 100ns = 1ms, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was really the only reason that TryLockConsole
existed in the first place? Wow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, this is actually a lot less scary of a change than I thought it would be haha
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