-
Notifications
You must be signed in to change notification settings - Fork 0
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
WebGL context loss handling #5
Conversation
Testing casesHere's a few tests I ran on Hyper. Not sure if there's currently a way to test this on the current demo page.
|
There's another approach to context-loss handling where the browser determines when to recreate contexts. I don't think this would work for us since we need control over the context recreation (to have LRU priority, for example) |
this._canvas.height = 0; | ||
|
||
if (!this._gl.isContextLost()) { | ||
console.log('Force context loss'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should happen automatically after dispose on the next GC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, however, we force it so that another context can be created before the next GC. If we don't force it here, Chrome might not notice there's an available context and could kill an active one.
What I observed is:
- open 20 tabs (the first 4 will lose their context)
- close the last 16 tabs
- close one more tab
- result: Chrome will warn that the max number of contexts has been reached
- expected: no warning
So, it seems like one of these is happening:
- GC is not firing during the closing of the 17 tabs
- GC isn't collecting the underlying webgl contexts (maybe it's lazy by design?)
- Someone's keeping a ref to this instance, preventing it from being GC
I'll grab a snapshot to make sure (3) is not happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm all webgl contexts are eventually GC'd. But we still need this to prevent Chrome from killing the oldest context (as opposed to the LRU one)
// This is unique to WebglRenderer because 'webgl2' contexts can become | ||
// invalidated at any moment, typically by creating other canvases with | ||
// 'webgl2' context, but also arbitrarily by the OS. | ||
private static _contextHelper = new class { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you pull this into WebglContextHelper.ts?
} | ||
|
||
public loseContext(): void { | ||
this._gl.getExtension('WEBGL_lose_context').loseContext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supported in Chrome? https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context/loseContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, that seems outdated. It's definitely supported on Chrome and Chromium: https://codesandbox.io/embed/xpjjjmn1oq?view=preview
Need the refactor to pull WebglContextHelper into its own class before merging this, this will involve breaking WebglContextHelper's dependence on private members of WebglRenderer. Also there are some lint issues that need to be fixed too. |
Closing off in favor of xtermjs#2253 |
This PR adds a helper class in
WebglRenderer
that manages webgl contexts. This is needed because:When a context is lost:
When a renderer is unpaused, if its context has been lost, we simply get rid of the whole renderer and ask
Terminal
to create a new one.Renderers are kept ordered by least-recently-used so that less used ones are killed first when resources are needed.
If one of the context that are being recovered was just lost, it means that we're currently at a resource limit so we need to kill a few paused renderers. if we fail to release other contexts (i.e. everything is unpaused), there's no point in trying to recover anything because we would be killing an unpaused renderers. In this case, just give up.
I used a class expression to have access to
WebglRenderer
private fields while keeping this whole thing encapsulated