Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Set memory order on slow atomics #6920

Merged
3 commits merged into from
Jul 17, 2020
Merged

Set memory order on slow atomics #6920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
824e07d
use acquire/release instead of seq_cst for single producer single con…
miniksa Jul 14, 2020
226cc95
use release/acquire over seq cst on the atomic flag in throttledfunc
miniksa Jul 14, 2020
fcbbd72
ignore std memory_order spelling.
miniksa Jul 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/patterns/patterns.txt
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I understand it properly, this one probably shouldn't be relaxed because you're checking two different atomics instead of just one (and hoping for consistency across both), right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be "relaxed" because the "release" would make prior writes from this thread visible to other threads who "acquire" _fWaiting's value. But in this case the only one "acquiring" its value is NotifyPaint(), which doesn't need any prior writes done by _ThreadProc, since the only thing NotifyPaint() does is write to _fNextFrameRequested (but it doesn't rely upon its value, nor does it rely on any other value).
(This information is supplied without liability. ⚖😄)


// 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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean... Technically you can shave off a bit time here on x86 as well. While x86 doesn't make a distinction between relaxed and release, it does invalidate caches during an acquire (and that way previously "released" data is being sync'd into your "acquiring" thread).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm just going to leave it for now. I'm still not 100% comfortable with all this "manual memory order" business. And you did say above that your relaxed recommendation was provided without liability. :P So if the liability is mine, I'd like to stay in my comfy place for right now and consider relaxed in the future.

{
SetEvent(_hEvent);
}
else
{
_fNextFrameRequested.store(true);
_fNextFrameRequested.store(true, std::memory_order_release);
}
}

Expand Down