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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
deb1c62
Improve responsiveness of conhost/ConPTY for large inputs
lhecker 75f2609
Fix test failure
lhecker f16c967
Revert unnecessary change
lhecker b68d004
Address feedback
lhecker 9a7462f
Revert src/host/_stream.cpp
lhecker 2bf1000
Address feedback
lhecker File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ | |
#include "../interactivity/inc/ServiceLocator.hpp" | ||
#include "../types/inc/convert.hpp" | ||
|
||
#include <til/ticket_lock.h> | ||
|
||
using Microsoft::Console::Interactivity::ServiceLocator; | ||
using Microsoft::Console::Render::BlinkingState; | ||
using Microsoft::Console::VirtualTerminal::VtIo; | ||
|
@@ -44,42 +46,52 @@ CONSOLE_INFORMATION::CONSOLE_INFORMATION() : | |
{ | ||
ZeroMemory((void*)&CPInfo, sizeof(CPInfo)); | ||
ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo)); | ||
InitializeCriticalSection(&_csConsoleLock); | ||
} | ||
|
||
CONSOLE_INFORMATION::~CONSOLE_INFORMATION() | ||
{ | ||
DeleteCriticalSection(&_csConsoleLock); | ||
} | ||
|
||
bool CONSOLE_INFORMATION::IsConsoleLocked() const | ||
// Access to thread-local variables is thread-safe, because each thread | ||
// gets its own copy of this variable with a default value of 0. | ||
// | ||
// Whenever we want to acquire the lock we increment recursionCount and on | ||
// each release we decrement it again. We can then make the lock safe for | ||
// reentrancy by only acquiring/releasing the lock if the recursionCount is 0. | ||
// In a sense, recursionCount is counting the actual function | ||
// call recursion depth of the caller. This works as long as | ||
// the caller makes sure to properly call Unlock() once for each Lock(). | ||
static thread_local ULONG recursionCount = 0; | ||
miniksa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
static til::ticket_lock lock; | ||
Comment on lines
+60
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
bool CONSOLE_INFORMATION::IsConsoleLocked() | ||
{ | ||
// The critical section structure's OwningThread field contains the ThreadId despite having the HANDLE type. | ||
// This requires us to hard cast the ID to compare. | ||
return _csConsoleLock.OwningThread == (HANDLE)GetCurrentThreadId(); | ||
return recursionCount != 0; | ||
} | ||
|
||
#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.") | ||
void CONSOLE_INFORMATION::LockConsole() | ||
{ | ||
EnterCriticalSection(&_csConsoleLock); | ||
} | ||
|
||
#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.") | ||
bool CONSOLE_INFORMATION::TryLockConsole() | ||
{ | ||
return !!TryEnterCriticalSection(&_csConsoleLock); | ||
// See description of recursionCount a few lines above. | ||
const auto rc = ++recursionCount; | ||
miniksa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
FAIL_FAST_IF(rc == 0); | ||
if (rc == 1) | ||
{ | ||
lock.lock(); | ||
} | ||
} | ||
|
||
#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.") | ||
void CONSOLE_INFORMATION::UnlockConsole() | ||
{ | ||
LeaveCriticalSection(&_csConsoleLock); | ||
// See description of recursionCount a few lines above. | ||
const auto rc = --recursionCount; | ||
FAIL_FAST_IF(rc == ULONG_MAX); | ||
if (rc == 0) | ||
{ | ||
lock.unlock(); | ||
} | ||
} | ||
|
||
ULONG CONSOLE_INFORMATION::GetCSRecursionCount() | ||
{ | ||
return _csConsoleLock.RecursionCount; | ||
return recursionCount; | ||
} | ||
|
||
// Routine Description: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.