Skip to content

Commit

Permalink
Set memory order on slow atomics (#6920)
Browse files Browse the repository at this point in the history
By default, the memory order on atomics is `seq_cst`. This is a relatively expensive ordering and it shows in situations where we're rapidly signaling a consumer to pick up something from a producer. I've instead attempted to switch these to `release` (producer) and `acquire` (consumer) to improve the performance of these signals. 

## Validation Steps Performed
- Run `time cat big.txt` and `time cat ls.txt` under VS Performance Profiler. 

## PR Checklist
* [x] Closes perf itch
* [x] I work here
* [x] Manual test
* [x] Documentation irrelevant.
* [x] Schema irrelevant.
* [x] Am core contributor.
  • Loading branch information
miniksa authored Jul 17, 2020
1 parent 1f8264d commit ea2bd42
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 8 deletions.
1 change: 1 addition & 0 deletions .github/actions/spell-check/patterns/patterns.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,4 @@ TestUtils::VerifyExpectedString\(tb, L"[^"]+"
Base64::s_(?:En|De)code\(L"[^"]+"
VERIFY_ARE_EQUAL\(L"[^"]+"
L"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789\+/"
std::memory_order_[\w]+
4 changes: 2 additions & 2 deletions src/cascadia/TerminalControl/ThrottledFunc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ ThrottledFunc<>::ThrottledFunc(ThrottledFunc::Func func, TimeSpan delay, CoreDis
// - <none>
void ThrottledFunc<>::Run()
{
if (_isRunPending.test_and_set())
if (_isRunPending.test_and_set(std::memory_order_acquire))
{
// already pending
return;
Expand All @@ -44,7 +44,7 @@ void ThrottledFunc<>::Run()
if (auto self{ weakThis.lock() })
{
timer.Stop();
self->_isRunPending.clear();
self->_isRunPending.clear(std::memory_order_release);
self->_func();
}
});
Expand Down
12 changes: 6 additions & 6 deletions src/renderer/base/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,17 +162,17 @@ DWORD WINAPI RenderThread::_ThreadProc()
{
WaitForSingleObject(_hPaintEnabledEvent, INFINITE);

if (!_fNextFrameRequested.exchange(false))
if (!_fNextFrameRequested.exchange(false, std::memory_order_acq_rel))
{
// <--
// If `NotifyPaint` is called at this point, then it will not
// set the event because `_fWaiting` is not `true` yet so we have
// to check again below.

_fWaiting.store(true);
_fWaiting.store(true, std::memory_order_release);

// check again now (see comment above)
if (!_fNextFrameRequested.exchange(false))
if (!_fNextFrameRequested.exchange(false, std::memory_order_acq_rel))
{
// Wait until a next frame is requested.
WaitForSingleObject(_hEvent, INFINITE);
Expand All @@ -193,7 +193,7 @@ DWORD WINAPI RenderThread::_ThreadProc()
// expensive operation, we should reset the event to not render
// again if nothing changed.

_fWaiting.store(false);
_fWaiting.store(false, std::memory_order_release);

// see comment above
ResetEvent(_hEvent);
Expand All @@ -218,13 +218,13 @@ DWORD WINAPI RenderThread::_ThreadProc()

void RenderThread::NotifyPaint()
{
if (_fWaiting.load())
if (_fWaiting.load(std::memory_order_acquire))
{
SetEvent(_hEvent);
}
else
{
_fNextFrameRequested.store(true);
_fNextFrameRequested.store(true, std::memory_order_release);
}
}

Expand Down

0 comments on commit ea2bd42

Please sign in to comment.