Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Let custom editors retain context and shut them down only after inactivity or memory pressure #113507

Closed
Tyriar opened this issue Dec 28, 2020 · 5 comments
Assignees
Labels
custom-editors Custom editor API (webview based editors) feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Dec 28, 2020

retainContextWhenHidden can be used for custom editors to prevent the webview from being torn down and set back up again. This is a big UX win as there will no longer be the short delay in launching the webview, unfortunately I think it will cause that memory to be held onto indefinitely. I'm proposing we have a middle ground between retainContextWhenHidden true and false where the editor acts more like a browser where the context will go away only on certain conditions like:

  • Memory pressure
  • x minutes have elapsed
  • x newer tabs have been opened
  • etc.

The specific cases could be up to VS Code, I'd just like retainContextWhenHidden: true-like behavior shortly after the tab has been active.

I don't even think you would need to wake up the webview to get it to save state because that would already be handled by backupCustomDocument.

@mjbvz
Copy link
Collaborator

mjbvz commented Jan 4, 2021

I don't think memory pressure is going to be viable (specifically on the web) but number of other opened documents is an interesting idea

@mjbvz mjbvz added this to the Backlog milestone Jan 4, 2021
@mjbvz mjbvz added the feature-request Request for new features or functionality label Jan 4, 2021
@Tyriar
Copy link
Member Author

Tyriar commented Jan 4, 2021

Being able to shut it down when I choose would also be good to have, I could shut then down after the backup has definitely occurred (forcing large images to backup since it's not in the foreground).

@mjbvz
Copy link
Collaborator

mjbvz commented Jan 8, 2021

I like the idea of letting extensions telling us the webview content is safe to dispose of. I think that would work well for cases where the webview needs to sync lots of data back to the extension host

How about something like this inside the webview itself:

// in webview

const vscode = acquireVsCodeApi();

...

if (content saved and no changes have been made to it) {
    // Setting this flag would indicate that a webview marked with `retainContextWhenHidden` could
   // be reclaimed by VS Code
    vscode.isPersisted = true;
}

I still need to think a little more about if this should happen in the webview or in the extension host. I think there's a potential for data loss in both cases.

If set inside the webview:

  • The extension sets isPersisted in the webview
  • We then have to tell VS Code that it can destroy the webview using an async message
  • Before that message is received, the user somehow makes an edit in the webview that could cause isPersisted to become false (hopefully not as likely since the webview would not be visible, but it could still be receiving messages)

If set in the extension host:

  • The extension sets isPersisted for a extension host webview object
  • We then have to tell core VS Code that it can reclaim the webview using an async message
  • Before this happens, the user makes an edit in the webview

@Tyriar
Copy link
Member Author

Tyriar commented Jan 8, 2021

I think something like that would work great, combined with WebviewPanel.onDidChangeViewState to track when the webview is backgrounded.

I don't share the concern so much for data loss as I would only be setting this when the webview is not visible. For example, even for small images where backups happen fine very quickly, I could still send a message to the hidden webview when it gets hidden and it would set it at that point without needing to do a backup. The name I would use though which aligns closer with how I would use it would be something like isSafeToDestroy, mustRetainContext, allowContextReclaim, etc.

@vscodenpa
Copy link

We closed this issue because we don't plan to address it in the foreseeable future. If you disagree and feel that this issue is crucial: we are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding, and happy coding!

@vscodenpa vscodenpa closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
custom-editors Custom editor API (webview based editors) feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

No branches or pull requests

3 participants