2019-05-03 00:29:04 +02:00
|
|
|
|
// Copyright (c) Microsoft Corporation.
|
|
|
|
|
// Licensed under the MIT license.
|
|
|
|
|
|
|
|
|
|
#include "precomp.h"
|
|
|
|
|
|
|
|
|
|
#include "thread.hpp"
|
|
|
|
|
|
|
|
|
|
#pragma hdrstop
|
|
|
|
|
|
|
|
|
|
using namespace Microsoft::Console::Render;
|
|
|
|
|
|
|
|
|
|
RenderThread::RenderThread() :
|
|
|
|
|
_pRenderer(nullptr),
|
2019-05-28 18:56:36 +02:00
|
|
|
|
_hThread(nullptr),
|
|
|
|
|
_hEvent(nullptr),
|
|
|
|
|
_hPaintCompletedEvent(nullptr),
|
2019-05-03 00:29:04 +02:00
|
|
|
|
_fKeepRunning(true),
|
Fix RenderThread's notify mechanism (#4698)
## 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. ✔️
2020-02-27 19:29:21 +01:00
|
|
|
|
_hPaintEnabledEvent(nullptr),
|
|
|
|
|
_fNextFrameRequested(false),
|
|
|
|
|
_fWaiting(false)
|
2019-05-03 00:29:04 +02:00
|
|
|
|
{
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
RenderThread::~RenderThread()
|
|
|
|
|
{
|
2019-05-28 18:56:36 +02:00
|
|
|
|
if (_hThread)
|
2019-05-03 00:29:04 +02:00
|
|
|
|
{
|
|
|
|
|
_fKeepRunning = false; // stop loop after final run
|
2020-04-14 22:11:47 +02:00
|
|
|
|
EnablePainting(); // if we want to get the last frame out, we need to make sure it's enabled
|
2019-05-03 00:29:04 +02:00
|
|
|
|
SignalObjectAndWait(_hEvent, _hThread, INFINITE, FALSE); // signal final paint and wait for thread to finish.
|
|
|
|
|
|
|
|
|
|
CloseHandle(_hThread);
|
2019-05-28 18:56:36 +02:00
|
|
|
|
_hThread = nullptr;
|
2019-05-03 00:29:04 +02:00
|
|
|
|
}
|
|
|
|
|
|
2019-05-28 18:56:36 +02:00
|
|
|
|
if (_hEvent)
|
2019-05-03 00:29:04 +02:00
|
|
|
|
{
|
|
|
|
|
CloseHandle(_hEvent);
|
2019-05-28 18:56:36 +02:00
|
|
|
|
_hEvent = nullptr;
|
2019-05-03 00:29:04 +02:00
|
|
|
|
}
|
|
|
|
|
|
2019-05-28 18:56:36 +02:00
|
|
|
|
if (_hPaintEnabledEvent)
|
2019-05-03 00:29:04 +02:00
|
|
|
|
{
|
|
|
|
|
CloseHandle(_hPaintEnabledEvent);
|
2019-05-28 18:56:36 +02:00
|
|
|
|
_hPaintEnabledEvent = nullptr;
|
2019-05-03 00:29:04 +02:00
|
|
|
|
}
|
|
|
|
|
|
2019-05-28 18:56:36 +02:00
|
|
|
|
if (_hPaintCompletedEvent)
|
2019-05-03 00:29:04 +02:00
|
|
|
|
{
|
|
|
|
|
CloseHandle(_hPaintCompletedEvent);
|
2019-05-28 18:56:36 +02:00
|
|
|
|
_hPaintCompletedEvent = nullptr;
|
2019-05-03 00:29:04 +02:00
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// Method Description:
|
|
|
|
|
// - Create all of the Events we'll need, and the actual thread we'll be doing
|
|
|
|
|
// work on.
|
|
|
|
|
// Arguments:
|
|
|
|
|
// - pRendererParent: the IRenderer that owns this thread, and which we should
|
|
|
|
|
// trigger frames for.
|
|
|
|
|
// Return Value:
|
|
|
|
|
// - S_OK if we succeeded, else an HRESULT corresponding to a failure to create
|
|
|
|
|
// an Event or Thread.
|
2019-06-11 22:27:09 +02:00
|
|
|
|
[[nodiscard]] HRESULT RenderThread::Initialize(IRenderer* const pRendererParent) noexcept
|
2019-05-03 00:29:04 +02:00
|
|
|
|
{
|
|
|
|
|
_pRenderer = pRendererParent;
|
|
|
|
|
|
|
|
|
|
HRESULT hr = S_OK;
|
|
|
|
|
// Create event before thread as thread will start immediately.
|
|
|
|
|
if (SUCCEEDED(hr))
|
|
|
|
|
{
|
|
|
|
|
HANDLE hEvent = CreateEventW(nullptr, // non-inheritable security attributes
|
2019-06-11 22:27:09 +02:00
|
|
|
|
FALSE, // auto reset event
|
|
|
|
|
FALSE, // initially unsignaled
|
|
|
|
|
nullptr // no name
|
|
|
|
|
);
|
2019-05-03 00:29:04 +02:00
|
|
|
|
|
|
|
|
|
if (hEvent == nullptr)
|
|
|
|
|
{
|
|
|
|
|
hr = HRESULT_FROM_WIN32(GetLastError());
|
|
|
|
|
}
|
|
|
|
|
else
|
|
|
|
|
{
|
|
|
|
|
_hEvent = hEvent;
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
if (SUCCEEDED(hr))
|
|
|
|
|
{
|
|
|
|
|
HANDLE hPaintEnabledEvent = CreateEventW(nullptr,
|
2019-06-11 22:27:09 +02:00
|
|
|
|
TRUE, // manual reset event
|
|
|
|
|
FALSE, // initially signaled
|
2019-05-03 00:29:04 +02:00
|
|
|
|
nullptr);
|
|
|
|
|
|
|
|
|
|
if (hPaintEnabledEvent == nullptr)
|
|
|
|
|
{
|
|
|
|
|
hr = HRESULT_FROM_WIN32(GetLastError());
|
|
|
|
|
}
|
|
|
|
|
else
|
|
|
|
|
{
|
|
|
|
|
_hPaintEnabledEvent = hPaintEnabledEvent;
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
if (SUCCEEDED(hr))
|
|
|
|
|
{
|
|
|
|
|
HANDLE hPaintCompletedEvent = CreateEventW(nullptr,
|
2019-06-11 22:27:09 +02:00
|
|
|
|
TRUE, // manual reset event
|
|
|
|
|
TRUE, // initially signaled
|
2019-05-03 00:29:04 +02:00
|
|
|
|
nullptr);
|
|
|
|
|
|
|
|
|
|
if (hPaintCompletedEvent == nullptr)
|
|
|
|
|
{
|
|
|
|
|
hr = HRESULT_FROM_WIN32(GetLastError());
|
|
|
|
|
}
|
|
|
|
|
else
|
|
|
|
|
{
|
|
|
|
|
_hPaintCompletedEvent = hPaintCompletedEvent;
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
if (SUCCEEDED(hr))
|
|
|
|
|
{
|
2019-06-11 22:27:09 +02:00
|
|
|
|
HANDLE hThread = CreateThread(nullptr, // non-inheritable security attributes
|
|
|
|
|
0, // use default stack size
|
2019-05-03 00:29:04 +02:00
|
|
|
|
s_ThreadProc,
|
|
|
|
|
this,
|
2019-06-11 22:27:09 +02:00
|
|
|
|
0, // create immediately
|
|
|
|
|
nullptr // we don't need the thread ID
|
|
|
|
|
);
|
2019-05-03 00:29:04 +02:00
|
|
|
|
|
|
|
|
|
if (hThread == nullptr)
|
|
|
|
|
{
|
|
|
|
|
hr = HRESULT_FROM_WIN32(GetLastError());
|
|
|
|
|
}
|
|
|
|
|
else
|
|
|
|
|
{
|
|
|
|
|
_hThread = hThread;
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
return hr;
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
DWORD WINAPI RenderThread::s_ThreadProc(_In_ LPVOID lpParameter)
|
|
|
|
|
{
|
|
|
|
|
RenderThread* const pContext = static_cast<RenderThread*>(lpParameter);
|
|
|
|
|
|
|
|
|
|
if (pContext != nullptr)
|
|
|
|
|
{
|
|
|
|
|
return pContext->_ThreadProc();
|
|
|
|
|
}
|
|
|
|
|
else
|
|
|
|
|
{
|
|
|
|
|
return (DWORD)E_INVALIDARG;
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
DWORD WINAPI RenderThread::_ThreadProc()
|
|
|
|
|
{
|
|
|
|
|
while (_fKeepRunning)
|
|
|
|
|
{
|
|
|
|
|
WaitForSingleObject(_hPaintEnabledEvent, INFINITE);
|
2020-02-14 22:58:28 +01:00
|
|
|
|
|
Fix RenderThread's notify mechanism (#4698)
## 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. ✔️
2020-02-27 19:29:21 +01:00
|
|
|
|
if (!_fNextFrameRequested.exchange(false))
|
2020-02-14 22:58:28 +01:00
|
|
|
|
{
|
Fix RenderThread's notify mechanism (#4698)
## 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. ✔️
2020-02-27 19:29:21 +01:00
|
|
|
|
// <--
|
|
|
|
|
// 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);
|
2020-02-14 22:58:28 +01:00
|
|
|
|
}
|
2019-05-03 00:29:04 +02:00
|
|
|
|
|
|
|
|
|
ResetEvent(_hPaintCompletedEvent);
|
|
|
|
|
|
|
|
|
|
LOG_IF_FAILED(_pRenderer->PaintFrame());
|
|
|
|
|
|
|
|
|
|
SetEvent(_hPaintCompletedEvent);
|
|
|
|
|
|
|
|
|
|
// extra check before we sleep since it's a "long" activity, relatively speaking.
|
|
|
|
|
if (_fKeepRunning)
|
|
|
|
|
{
|
|
|
|
|
Sleep(s_FrameLimitMilliseconds);
|
|
|
|
|
}
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
return S_OK;
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
void RenderThread::NotifyPaint()
|
|
|
|
|
{
|
Fix RenderThread's notify mechanism (#4698)
## 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. ✔️
2020-02-27 19:29:21 +01:00
|
|
|
|
if (_fWaiting.load())
|
2020-02-14 22:58:28 +01:00
|
|
|
|
{
|
Fix RenderThread's notify mechanism (#4698)
## 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. ✔️
2020-02-27 19:29:21 +01:00
|
|
|
|
SetEvent(_hEvent);
|
|
|
|
|
}
|
|
|
|
|
else
|
|
|
|
|
{
|
|
|
|
|
_fNextFrameRequested.store(true);
|
2020-02-14 22:58:28 +01:00
|
|
|
|
}
|
2019-05-03 00:29:04 +02:00
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
void RenderThread::EnablePainting()
|
|
|
|
|
{
|
|
|
|
|
SetEvent(_hPaintEnabledEvent);
|
2020-04-14 22:11:47 +02:00
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
void RenderThread::DisablePainting()
|
|
|
|
|
{
|
|
|
|
|
ResetEvent(_hPaintEnabledEvent);
|
2019-05-03 00:29:04 +02:00
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
void RenderThread::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs)
|
|
|
|
|
{
|
|
|
|
|
// When rendering takes place via DirectX, and a console application
|
|
|
|
|
// currently owns the screen, and a new console application is launched (or
|
|
|
|
|
// the user switches to another console application), the new application
|
|
|
|
|
// cannot take over the screen until the active one relinquishes it. This
|
|
|
|
|
// blocking mechanism goes as follows:
|
|
|
|
|
//
|
|
|
|
|
// 1. The console input thread of the new console application connects to
|
|
|
|
|
// ConIoSrv;
|
|
|
|
|
// 2. While servicing the new connection request, ConIoSrv sends an event to
|
|
|
|
|
// the active application letting it know that it has lost focus;
|
|
|
|
|
// 3.1 ConIoSrv waits for a reply from the client application;
|
|
|
|
|
// 3.2 Meanwhile, the active application receives the focus event and calls
|
2020-02-10 21:40:01 +01:00
|
|
|
|
// this method, waiting for the current paint operation to
|
2019-05-03 00:29:04 +02:00
|
|
|
|
// finish.
|
|
|
|
|
//
|
|
|
|
|
// This means that the new application is waiting on the connection request
|
|
|
|
|
// reply from ConIoSrv, ConIoSrv is waiting on the active application to
|
|
|
|
|
// acknowledge the lost focus event to reply to the new application, and the
|
|
|
|
|
// console input thread in the active application is waiting on the renderer
|
|
|
|
|
// thread to finish its current paint operation.
|
|
|
|
|
//
|
|
|
|
|
// Question: what should happen if the wait on the paint operation times
|
|
|
|
|
// out?
|
|
|
|
|
//
|
|
|
|
|
// There are three options:
|
|
|
|
|
//
|
|
|
|
|
// 1. On timeout, the active console application could reply with an error
|
|
|
|
|
// message and terminate itself, effectively relinquishing control of the
|
|
|
|
|
// display;
|
|
|
|
|
//
|
|
|
|
|
// 2. ConIoSrv itself could time out on waiting for a reply, and forcibly
|
|
|
|
|
// terminate the active console application;
|
|
|
|
|
//
|
|
|
|
|
// 3. Let the wait time out and let the user deal with it. Because the wait
|
|
|
|
|
// occurs on a single iteration of the renderer thread, it seemed to me that
|
|
|
|
|
// the likelihood of failure is extremely small, especially since the client
|
|
|
|
|
// console application that the active conhost instance is servicing has no
|
|
|
|
|
// say over what happens in the renderer thread, only by proxy. Thus, the
|
|
|
|
|
// chance of failure (timeout) is minimal and since the OneCoreUAP console
|
|
|
|
|
// is not a massively used piece of software, it didn’t seem that it would
|
|
|
|
|
// be a good use of time to build the requisite infrastructure to deal with
|
|
|
|
|
// a timeout here, at least not for now. In case of a timeout DirectX will
|
|
|
|
|
// catch the mistake of a new application attempting to acquire the display
|
|
|
|
|
// while another one still owns it and will flag it as a DWM bug. Right now,
|
|
|
|
|
// the active application will wait one second for the paint operation to
|
|
|
|
|
// finish.
|
|
|
|
|
//
|
|
|
|
|
// TODO: MSFT: 11833883 - Determine action when wait on paint operation via
|
|
|
|
|
// DirectX on OneCoreUAP times out while switching console
|
|
|
|
|
// applications.
|
|
|
|
|
|
|
|
|
|
ResetEvent(_hPaintEnabledEvent);
|
|
|
|
|
WaitForSingleObject(_hPaintCompletedEvent, dwTimeoutMs);
|
|
|
|
|
}
|