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.

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

Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI #5353

Merged
4 commits merged into from
Apr 14, 2020

Conversation

DHowett-MSFT
Copy link
Contributor

Summary of the Pull Request

Renderer: Add support for backoff and auto-disable on failed retry

This commit introduces a backoff (150ms * number of tries) to the
renderer's retry logic (introduced in #2830). It also changes the
FAIL_FAST to a less globally-harmful render thread disable, so that we
stop blowing up any application hosting a terminal when the graphics
driver goes away.

In addition, it adds a callback that a Renderer consumer can use to
determine when the renderer has failed, and a public method to kick it
back into life.

Fixes #5340.

This PR also wires up TermControl so that it shows some UI when the renderer tastes clay.

image

image

PR Checklist

Validation Steps Performed

I tested this by dropping the number of retries to 1 and forcing a TDR while doing wsl cmatrix -u0. It picked up exactly where it left off.

As a bonus, you can actually still type into the terminal when it's graphically suspended (and exit still works.). The block is entirely graphical.

This commit introduces a backoff (150ms * number of tries) to the
renderer's retry logic (introduced in #2830). It also changes the
FAIL_FAST to a less globally-harmful render thread disable, so that we
stop blowing up any application hosting a terminal when the graphics
driver goes away.

In addition, it adds a callback that a Renderer consumer can use to
determine when the renderer _has_ failed, and a public method to kick it
back into life.

Fixes #5340.
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Only minor questions tbh

src/cascadia/TerminalControl/TermControl.xaml Show resolved Hide resolved
}
// Add a bit of backoff.
// Sleep 150ms, 300ms, 450ms before failing out and disabling the renderer.
Sleep(150 * (maxRetriesForRenderEngine - tries));
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for the backoff like this? just to give it another chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under the idea that the graphics driver probably went down for a real reason, and that restarting the card might take some time if that's what it needs to do.

}
// Add a bit of backoff.
// Sleep 150ms, 300ms, 450ms before failing out and disabling the renderer.
Sleep(150 * (maxRetriesForRenderEngine - tries));
Copy link
Member

Choose a reason for hiding this comment

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

Should 150 be a constant somewhere? 🤷‍♂️

src/renderer/base/renderer.cpp Show resolved Hide resolved
@zadjii-msft
Copy link
Member

@msftbot make sure @miniksa signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 14, 2020
@ghost
Copy link

ghost commented Apr 14, 2020

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@zadjii-msft zadjii-msft added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Apr 14, 2020
@miniksa
Copy link
Member

miniksa commented Apr 14, 2020

Looks alright, but I'd like to see @zadjii-msft's questions answered and @cinnamon-msft give this a pass for how it looks before we commit it.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

alright I'm cool with this

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

Sure ok.

@DHowett-MSFT
Copy link
Contributor Author

@msftbot merge this in 2 minutes

@ghost
Copy link

ghost commented Apr 14, 2020

Hello @DHowett-MSFT!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa
  • I won't merge this pull request until after the UTC date Tue, 14 Apr 2020 19:56:12 GMT, which is in 2 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit e61968c into master Apr 14, 2020
@ghost ghost deleted the dev/duhowett/eyes_off_the_prize branch April 14, 2020 20:11
DHowett-MSFT pushed a commit that referenced this pull request Apr 14, 2020
… UI (#5353)

Renderer: Add support for backoff and auto-disable on failed retry

This commit introduces a backoff (150ms * number of tries) to the
renderer's retry logic (introduced in #2830). It also changes the
FAIL_FAST to a less globally-harmful render thread disable, so that we
stop blowing up any application hosting a terminal when the graphics
driver goes away.

In addition, it adds a callback that a Renderer consumer can use to
determine when the renderer _has_ failed, and a public method to kick it
back into life.

Fixes #5340.

This PR also wires up TermControl so that it shows some UI when the renderer tastes clay.

![image](https://user-images.githubusercontent.com/14316954/79266118-f073f680-7e4b-11ea-8b96-5588a13aff3b.png)

![image](https://user-images.githubusercontent.com/14316954/79266125-f36ee700-7e4b-11ea-9314-4280e9149461.png)

* [x] Closes #5340
* [x] cla
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already.

I tested this by dropping the number of retries to 1 and forcing a TDR while doing `wsl cmatrix -u0`. It picked up exactly where it left off.

As a bonus, you can actually still type into the terminal when it's graphically suspended (and `exit` still works.). The block is _entirely graphical_.
DHowett-MSFT pushed a commit that referenced this pull request Apr 21, 2020
Improve wide glyph support in UIA (GH-4946)
Add enhanced key support for ConPty (GH-5021)
Set DxRenderer non-text alias mode (GH-5149)
Reduce CursorChanged Events for Accessibility (GH-5196)
Add more object ID tracing for Accessibility (GH-5215)
Add SS3 cursor key encoding to ConPty (GH-5383)
UIA: Prevent crash from invalid UTR endpoint comparison (GH-5399)
Make CodepointWidthDetector::GetWidth faster (CC-3727)
add til::math, use it for float conversions to point, size (GH-5150)
Add support for renderer backoff, don't FAIL_FAST on 3x failures, add UI (GH-5353)
Fix a deadlock and a bounding rects issue in UIA (GH-5385)
Don't duplicate spaces from potentially-wrapped EOL-deferred lines (GH-5398)
Reimplement the VT tab stop functionality (CC-5173)
Clamp parameter values to a maximum of 32767. (CC-5200)
Prevent the cursor type being reset when changing the visibility (CC-5251)
Make RIS switch back to the main buffer (CC-5248)
Add support for the DSR-OS operating status report (CC-5300)
Update the virtual bottom location if the cursor moves below it (CC-5317)
ci: run spell check in CI, fix remaining issues (CC-4799) (CC-5352)
Set Cascadia Code as default font (GH-5121)
Show a double width cursor for double width characters (GH-5319)
Delegate all character input to the character event handler (CC-4192)
Update til::bitmap to use dynamic_bitset<> + libpopcnt (GH-5092)
Merged PR 4465022: [Git2Git] Merged PR 4464559: Console: Ingest OSS changes up to e055079
Correct scrolling invalidation region for tmux in pty w/ bitmap (GH-5122)
Render row-by-row instead of invalidating entire screen (GH-5185)
Make conechokey use ReadConsoleInputW by default (GH-5148)
Manually pass mouse wheel messages to TermControls (GH-5131)
This fixes C-M-space for WSL but not for Win32, but I'm not sure there's a problem in Win32 quite yet. (GH-5208)
Fix copying wrapped lines by implementing better scrolling (GH-5181)
Emit lines wrapped due to spaces at the end correctly (GH-5294)
Remove unneeded whitespace (CC-5162)
@ghost
Copy link

ghost commented Apr 22, 2020

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in render engine due to directx device hanging up
5 participants