maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix RenderThread's notify mechanism (#4698)

Browse source 
## Summary of the Pull Request

Fix a bug where the `Renderer::PaintFrame` method:
1. is not called until the next `RenderThread::NotifyThread` call but needs to be called because there the terminal was updated (theoretical bug)
2. is called twice but needs to be called only once (verified bug)

## References

The bug was introduced by #3511.

## PR Checklist
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA

## Detailed Description of the Pull Request / Additional comments

### Before

#### First bug

In the original code, `_fNextFrameRequested` is set to `true` in render thread because `std::atomic_flag::test_and_set` is called.

This is wrong because it means that the render thread will render the terminal again even if there is no change after the last render.

I think the the goal was to load the boolean value for `_fNextFrameRequested` to check whether the thread should sleep or not.

The problem is that there is no method on `std::atomic_flag` to load its boolean value. I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value. I guess that this was believed to be equivalent to just a simple load, without doing any change to the value because it restores it at the end.

But it's not: this is dangerous because if the value is changed to `true` between the call to `std::atomic_flag::test_and_set` and the call to `std::atomic_flag::clear`, then the value ends up being `false` at the end which is wrong because we don't want to change it! And if that value ends up being `false`, it means that we miss a render because we will wait on `_hEvent` during the next iteration on the render thread.

Well actually, here, this not even a problem because when that code is ran, `_fPainting` is `false` which means that the other thread that modifies the `_fNextFrameRequested` value through `RenderThread::NotifyPaint` will not actually modify `_fNextFrameRequested` but rather call `SetEvent` (see the method's body).

But wait! There is a problem there too! `std::atomic_flag::test_and_set` is called for `_fPainting` which sets its value to `true`. It was probably unintended. So actually, the next call to `RenderThread::NotifyPaint` _will_ end up modifying `_fNextFrameRequested` which means that the data race I was talking about _might_ happen!

#### Second bug

Let's go back a little bit in my explanation. I was talking about the fact that:

> I guess what happened was that the "solution" that was found was to use `std::atomic_flag::test_and_set`, followed by `std::atomic_flag::clear` if the value was `false` originally (if `std::atomic_flag::test_and_set` returned `false`) to restore the original value.

The problem is that the reverse was done in the implementation: `std::atomic_flag::clear` is called if the value was _`true`_ originally!

So at this point, if the value of `_fNextFrameRequested` was `false`, then `std::atomic_flag::test_and_set` sets its is set to `true` and returns `false`. So for the next iteration, `_fNextFrameRequested` is `true` and the render thread will re-render but that was not needed.

### After

I used `std::atomic<bool>` instead of `std::atomic_flag` for `_fNextFrameRequested` and the other atomic field because it has a `load` and a `store` method so we can actually load the value without changing it.

I also replaced `_fPainting` by `_fWaiting`, which is basically the opposite of `_fPainting` but stays `true` for a little shorter than `_fPainting` would stay `false`. Indeed, I think that it makes more sense to directly wrap/scope _just_ the call to `WaitForSingleObject` by setting my atomic variable to `true` _just_ before and to `false` _just_ after because:
* It makes more sense while you're reading the code: it's easier IMO to understand what the purpose of `_fWaiting` is (that is, to call `SetEvent` from `RenderThread::NotifyPaint` if it's `true`).
* It's probably a tiny bit better for performance because it will become `true` for a little shorter which means less calls to `SetEvent`.

#### Warning

I don't really understand [std::memory_order](https://en.cppreference.com/w/cpp/atomic/memory_order)s.

So I used the default one (`std::memory_order_seq_cst`) which is the safest.

I believe that if no read or write are reordered in the two threads (`RenderThread::NotifyPaint` and `RenderThread::_ThreadProc`), then the code I wrote will behave correctly.

I think that `std::memory_order_seq_cst` enforces that so it should be fine, but I'm not sure.

## Validation Steps Performed

**I tried to reproduce the second bug that I described in the first section of this PR.**

I put a breakpoint on `RenderThread::NotifyPaint` and on `Renderer::PaintFrame`. Initially they are disabled. Then I ran the terminal in Release mode, waited a bit for the prompt to display and the cursor to start blinking. Then I enabled the breakpoints.

### Before

Each `RenderThread::NotifyPaint` is followed by 2 `Renderer::PaintFrame` calls. ❌ 

### After

Each `RenderThread::NotifyPaint` is followed by 1 `Renderer::PaintFrame` call. ✔️
This commit is contained in:
greg904 2020-02-27 19:29:21 +01:00 committed by GitHub
parent e5182fb3e8
commit 2f60cf0e91
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 45 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

64
src/renderer/base/thread.cpp
View file

@ -15,10 +15,10 @@ RenderThread::RenderThread() :
_hEvent(nullptr),
_hPaintCompletedEvent(nullptr),
_fKeepRunning(true),
_hPaintEnabledEvent(nullptr)
_hPaintEnabledEvent(nullptr),
_fNextFrameRequested(false),
_fWaiting(false)
{
_fNextFrameRequested.clear();
_fPainting.clear();
}
RenderThread::~RenderThread()
@ -161,20 +161,45 @@ DWORD WINAPI RenderThread::_ThreadProc()
{
WaitForSingleObject(_hPaintEnabledEvent, INFINITE);
// Skip waiting if next frame is requested.
if (_fNextFrameRequested.test_and_set(std::memory_order_relaxed))
if (!_fNextFrameRequested.exchange(false))
{
_fNextFrameRequested.clear(std::memory_order_relaxed);
}
else
{
WaitForSingleObject(_hEvent, INFINITE);
// <--
// 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);
// check again now (see comment above)
if (!_fNextFrameRequested.exchange(false))
{
// Wait until a next frame is requested.
WaitForSingleObject(_hEvent, INFINITE);
}
// <--
// If `NotifyPaint` is called at this point, then it _will_ set
// the event because `_fWaiting` is `true`, but we're not waiting
// anymore!
// This can probably happen quite often: imagine a scenario where
// we are waiting, and the terminal calls `NotifyPaint` twice
// very quickly.
// In that case, both calls might end up calling `SetEvent`. The
// first one will resume this thread and the second one will
// `SetEvent` the event. So the next time we wait, the event will
// already be set and we won't actually wait.
// Because it can happen often, and because rendering is an
// expensive operation, we should reset the event to not render
// again if nothing changed.
_fWaiting.store(false);
// see comment above
ResetEvent(_hEvent);
}
ResetEvent(_hPaintCompletedEvent);
_fPainting.test_and_set(std::memory_order_acquire);
LOG_IF_FAILED(_pRenderer->PaintFrame());
SetEvent(_hPaintCompletedEvent);
@ -184,8 +209,6 @@ DWORD WINAPI RenderThread::_ThreadProc()
{
Sleep(s_FrameLimitMilliseconds);
}
_fPainting.clear(std::memory_order_release);
}
return S_OK;
@ -193,15 +216,14 @@ DWORD WINAPI RenderThread::_ThreadProc()
void RenderThread::NotifyPaint()
{
// If we are currently painting a frame, set _fNextFrameRequested flag
// to indicate we want to paint next frame immediately.
if (_fPainting.test_and_set(std::memory_order_acquire))
if (_fWaiting.load())
{
_fNextFrameRequested.test_and_set(std::memory_order_relaxed);
return;
SetEvent(_hEvent);
}
else
{
_fNextFrameRequested.store(true);
}
SetEvent(_hEvent);
}
void RenderThread::EnablePainting()

4
src/renderer/base/thread.hpp
View file

@ -47,7 +47,7 @@ namespace Microsoft::Console::Render
IRenderer* _pRenderer; // Non-ownership pointer
bool _fKeepRunning;
std::atomic_flag _fNextFrameRequested;
std::atomic_flag _fPainting;
std::atomic<bool> _fNextFrameRequested;
std::atomic<bool> _fWaiting;
};
}