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

Work around a (likely) optimizer issue with SetPixelShaderPath (#9092)

Browse source 
It appears as though the optimizer is generating a sequence of
instructions on x64 that results in a nonsense std::wstring_view being
passed to SetPixelShaderPath when it's converted from a winrt::hstring.

Initially, I suspected that the issue was in us caching `_settings`
before we broke off the coroutine to update settings on the UI thread. I
implemented a quick fix for this (applying values off the new settings
object while also storing it in the control instance), but it didn't
actually lead anywhere. I do think it's the right thing to do for code
health's sake. Pankaj already changed how this works in 1.7: we no
longer (ever) re-seat the `_settings` reference... we only ever change
its parentage. Whether this is right or wrong is not for this paragraph
to discuss.

Eventually, I started looking more closely at the time travel traces. It
seriously looks like the wstring_view is generated wrong to begin with.
The debugger points directly at `return { L"", 0 };` (which is correct),
but the values we get immediately on the other side of the call are
something like `{ 0x7FFFFFFF, 0 }` or `{ 0x0, 0x48454C4C }`.

I moved _just_ the call to SetPixelShaderPath into a separate function.
The bug miraculously disappeared when I marked it **noinline**. It
reappeared when the function was fully inlined.

To avoid any future issues, I moved the whole UI thread body of
UpdateSettings out into its own function, to be called only while on the
UI thread. This fixes the bug.

Closes #8723.
Closes #9064.

## Validation Steps Performed

I found a repro (update the settings file every 0.5 seconds and resize
the terminal wildly while it's doing so) that would trigger the bug
within ~10 seconds. It stopped doing so.
This commit is contained in:
Dustin L. Howett 2021-02-10 11:25:55 -08:00 committed by GitHub
parent 4014100b2d
commit 91b867102c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 79 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

130
src/cascadia/TerminalControl/TermControl.cpp
View file

@ -188,7 +188,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_autoScrollTimer.Interval(AutoScrollUpdateInterval);
_autoScrollTimer.Tick({ this, &TermControl::_UpdateAutoScroll });
_ApplyUISettings();
_ApplyUISettings(_settings);
}
// Method Description:
@ -274,6 +274,73 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
this->Focus(FocusState::Programmatic);
}
// Method Description:
// - Given new settings for this profile, applies the settings to the current terminal.
// - This method is separate from UpdateSettings because there is an apparent optimizer
// issue that causes one of our hstring -> wstring_view conversions to result in garbage,
// but only from a coroutine context. See GH#8723.
// - INVARIANT: This method must be called from the UI thread.
// Arguments:
// - newSettings: New settings values for the profile in this terminal.
// Return Value:
// - <none>
void TermControl::_UpdateSettingsOnUIThread(const IControlSettings& newSettings)
{
if (_closing)
{
return;
}
auto lock = _terminal->LockForWriting();
// First, store the new settings in the instance.
_settings = newSettings;
// Update our control settings
_ApplyUISettings(_settings);
// Update the terminal core with its new Core settings
_terminal->UpdateSettings(_settings);
if (!_initializedTerminal)
{
// If we haven't initialized, there's no point in continuing.
// Initialization will handle the renderer settings.
return;
}
// Update DxEngine settings under the lock
_renderEngine->SetSelectionBackground(_settings.SelectionBackground());
_renderEngine->SetRetroTerminalEffect(_settings.RetroTerminalEffect());
_renderEngine->SetPixelShaderPath(_settings.PixelShaderPath());
_renderEngine->SetForceFullRepaintRendering(_settings.ForceFullRepaintRendering());
_renderEngine->SetSoftwareRendering(_settings.SoftwareRendering());
switch (_settings.AntialiasingMode())
{
case TextAntialiasingMode::Cleartype:
_renderEngine->SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE_CLEARTYPE);
break;
case TextAntialiasingMode::Aliased:
_renderEngine->SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE_ALIASED);
break;
case TextAntialiasingMode::Grayscale:
default:
_renderEngine->SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE_GRAYSCALE);
break;
}
// Refresh our font with the renderer
const auto actualFontOldSize = _actualFont.GetSize();
_UpdateFont();
const auto actualFontNewSize = _actualFont.GetSize();
if (actualFontNewSize != actualFontOldSize)
{
_RefreshSizeUnderLock();
}
}
// Method Description:
// - Given new settings for this profile, applies the settings to the current terminal.
// Arguments:
@ -282,7 +349,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - <none>
winrt::fire_and_forget TermControl::UpdateSettings(IControlSettings newSettings)
{
_settings = newSettings;
auto weakThis{ get_weak() };
// Dispatch a call to the UI thread to apply the new settings to the
@ -292,49 +358,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
if (_closing)
{
co_return;
}
// Update our control settings
_ApplyUISettings();
// Update the terminal core with its new Core settings
_terminal->UpdateSettings(_settings);
auto lock = _terminal->LockForWriting();
// Update DxEngine settings under the lock
_renderEngine->SetSelectionBackground(_settings.SelectionBackground());
_renderEngine->SetRetroTerminalEffect(_settings.RetroTerminalEffect());
_renderEngine->SetPixelShaderPath(_settings.PixelShaderPath());
_renderEngine->SetForceFullRepaintRendering(_settings.ForceFullRepaintRendering());
_renderEngine->SetSoftwareRendering(_settings.SoftwareRendering());
switch (_settings.AntialiasingMode())
{
case TextAntialiasingMode::Cleartype:
_renderEngine->SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE_CLEARTYPE);
break;
case TextAntialiasingMode::Aliased:
_renderEngine->SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE_ALIASED);
break;
case TextAntialiasingMode::Grayscale:
default:
_renderEngine->SetAntialiasingMode(D2D1_TEXT_ANTIALIAS_MODE_GRAYSCALE);
break;
}
// Refresh our font with the renderer
const auto actualFontOldSize = _actualFont.GetSize();
_UpdateFont();
const auto actualFontNewSize = _actualFont.GetSize();
if (actualFontNewSize != actualFontOldSize)
{
_RefreshSizeUnderLock();
}
_UpdateSettingsOnUIThread(newSettings);
}
}
@ -384,21 +408,21 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - <none>
// Return Value:
// - <none>
void TermControl::_ApplyUISettings()
void TermControl::_ApplyUISettings(const IControlSettings& newSettings)
{
_InitializeBackgroundBrush();
COLORREF bg = _settings.DefaultBackground();
COLORREF bg = newSettings.DefaultBackground();
_BackgroundColorChanged(bg);
// Apply padding as swapChainPanel's margin
auto newMargin = _ParseThicknessFromPadding(_settings.Padding());
auto newMargin = _ParseThicknessFromPadding(newSettings.Padding());
SwapChainPanel().Margin(newMargin);
// Initialize our font information.
const auto fontFace = _settings.FontFace();
const short fontHeight = gsl::narrow_cast<short>(_settings.FontSize());
const auto fontWeight = _settings.FontWeight();
const auto fontFace = newSettings.FontFace();
const short fontHeight = gsl::narrow_cast<short>(newSettings.FontSize());
const auto fontWeight = newSettings.FontWeight();
// The font width doesn't terribly matter, we'll only be using the
// height to look it up
// The other params here also largely don't matter.
@ -410,12 +434,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// set TSF Foreground
Media::SolidColorBrush foregroundBrush{};
foregroundBrush.Color(static_cast<til::color>(_settings.DefaultForeground()));
foregroundBrush.Color(static_cast<til::color>(newSettings.DefaultForeground()));
TSFInputControl().Foreground(foregroundBrush);
TSFInputControl().Margin(newMargin);
// Apply settings for scrollbar
if (_settings.ScrollState() == ScrollbarState::Hidden)
if (newSettings.ScrollState() == ScrollbarState::Hidden)
{
// In the scenario where the user has turned off the OS setting to automatically hide scrollbars, the
// Terminal scrollbar would still be visible; so, we need to set the control's visibility accordingly to

3
src/cascadia/TerminalControl/TermControl.h
View file

@ -263,7 +263,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;
void _ApplyUISettings();
void _ApplyUISettings(const IControlSettings&);
void _UpdateSettingsOnUIThread(const IControlSettings& newSettings);
void _UpdateSystemParameterSettings() noexcept;
void _InitializeBackgroundBrush();
winrt::fire_and_forget _BackgroundColorChanged(const COLORREF color);