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.
This should happen automatically after dispose on the next GC?
There was a problem hiding this comment.
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.
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.
Could you pull this into WebglContextHelper.ts?
| } | ||
|
|
||
| public loseContext(): void { | ||
| this._gl.getExtension('WEBGL_lose_context').loseContext() |
There was a problem hiding this comment.
Not supported in Chrome? https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_lose_context/loseContext
There was a problem hiding this comment.
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
WebglRendererthat 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
Terminalto 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
WebglRendererprivate fields while keeping this whole thing encapsulated