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

findDocument API (work in progress) #80689

Closed
wants to merge 1 commit into from
Closed

findDocument API (work in progress) #80689

wants to merge 1 commit into from

Conversation

solomatov
Copy link
Contributor

This is a request for an API.
What do you think about?

I need this to prevent creating Document Indexes like this: https://github.com/microsoft/vscode/pull/80506/files#diff-1e4a9159b9b98358752ca990d9d2f545

Am I doing it correctly?
How can I test it?

@jrieken
Copy link
Member

jrieken commented Sep 11, 2019

This seems like a little sugar on top of vscode.workspace.textDocuments, e.g you can just use textDocuments.find(doc => doc.uri.toString() === someUri.toString())

@solomatov
Copy link
Contributor Author

@jrieken My understanding that this list can be very long, since once document is opened in an editor, it's kept there. So, if a user keeps opens vs.code for many days, and opens many documents, there might be thousands if not more text documents there.

See for example, ExtHostDocumentsAndEditors._documents, it's a map not a list.

What do you think?

@jrieken
Copy link
Member

jrieken commented Sep 11, 2019

I think it's a fair util but too much boilerplate for having this in the API. Yes, the list can get long but I'd say it rarely more than 100 items. VS Code manages documents and when they aren't being worked on they are closed to free resources.

@solomatov
Copy link
Contributor Author

@jrieken See #50874 This is the issue which referenced PR fixes. MD extension was opening a large number of MD documents, via an API, without loading them to the editor. Even though, they weren't opened in the editor, the were kept in memory and weren't thrown away which led to a performance problem.

Also, TextDocument, has a version field, which once incremented, should continue increasing monotonically, so to be consistent, vs.code can't remove TextDocuments.

What do you think?

@jrieken
Copy link
Member

jrieken commented Sep 11, 2019

and weren't thrown away which led to a performance problem.

Trust me, they will eventually be clean-up and they will be closed. That's why there is onDidCloseTextDocument-event. For documents requested from extensions it's controlled here:

export class BoundModelReferenceCollection {
and other document it's mostly driven by the UI, e.g editors that you see.

@solomatov
Copy link
Contributor Author

@jrieken And what about the version field? Is the version information gets thrown away? For example, in the referenced issue, the existence and monotonicity of increase of the version field is relied upon for cache consistency.

@jrieken
Copy link
Member

jrieken commented Sep 11, 2019

When a document is closed its gone. When the same file is opened again, the version id starts at its default again. That has been the case since ever and that's why it's vital to listen to onDidClose and onDidOpen event or to at least check TextDocument#isClosed.

@solomatov
Copy link
Contributor Author

@jrieken Ok, then this definitely isn't needed. Thanks for the clarifications!

@solomatov solomatov closed this Sep 11, 2019
@solomatov solomatov deleted the find-text-document branch September 11, 2019 16:10
@jrieken
Copy link
Member

jrieken commented Sep 11, 2019

Np. Thanks for getting back so quickly

@solomatov
Copy link
Contributor Author

@jrieken Thanks to you for the same as well)

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants