FS extension #3

Closed
wants to merge 7 commits into
from

Projects

None yet

5 participants

@sqs
Member
sqs commented Nov 2, 2016 edited

See newly added extension file.

@renfredxh @alexsaveliev @felixfbecker @slimsag: Does this address your needs? Would adopting this as the spec result in a performance improvement without significant additional implementation complexity?

Design goals:

  • Simplest protocol possible
  • Support servers that do not speak globs (e.g., be mostly backcompat with our existing Go LS, before we get around to updating it to prefetch/cache based on optimistic globbing)
  • Enable massive speedups for servers that do put in a bit of work to prefetch metadata or file contents for globs

TODOs:

  • Support the ** glob operator

Possible additions:

  • Allow fetching BOTH file contents and metadata in the same request/response? Would this help anyone?
  • Remove the size from the FileStats field - is that ever useful in the lang server case? It can be costly to read, since reading it for a git blob sometimes requires doing a lot of pointer derefs on disk.
@sqs
Member
sqs commented Nov 2, 2016

from @felixfbecker:

And yes, I need the **
Basically I will send you **/*.php and expect to get a list of TextDocumentIdentifier objects back with all PHP files in the workspace

@felixfbecker felixfbecker referenced this pull request Nov 2, 2016
Merged

workspace/files and textDocument/content extensions #4

1 of 1 task complete
@slimsag
Member
slimsag commented Nov 2, 2016

Looking at protocol.md.. am I right there are no (real) changes against what is currently in langserver-go ? If so, I do not understand this question:

Would adopting this as the spec result in a performance improvement without significant additional implementation complexity?

  • Many language servers need to traverse the entire workspace. With even a 3-5msec RTT between the client and server, this can become slow. Consider allowing language servers to provide globs to fetch information about multiple files at once, or similar.
    ...
  • Support servers that do not speak globs (e.g., be mostly backcompat with our existing Go LS, before we get around to updating it to prefetch/cache based on optimistic globbing)
  • Enable massive speedups for servers that do put in a bit of work to prefetch metadata or file contents for globs

I'm leery that correct globbing will always give us what we want. It's not easy to always make forward-thinking decisions about what files are needed without doing roundtrips to list files in directories (unless you're requesting ALL files, in which case all bets are off anyway).

I'll defer to others to chime in here, but I don't want us to use globbing as a misplaced 'batch operation' pattern that acts as a crutch to the real problem: our process has latency with the filesystem (and moving them closer together is the only real solution).

@sqs
Member
sqs commented Nov 2, 2016

@slimsag I meant, does it address your needs for improving performance of go-langserver?

Also compare to #4.

@alexsaveliev
Member

@sqs: Here are my comments

TS/JS needs are the following: We need the content of all ts, js, tsx, jsx, package.json, tsconfig.json, and jsconfig.json in the workspace.

With your proposal there are few topics to address:

  • optional exclusion of magic directories from processing. What if I want to omit .git or node_modules directory when fetching files? This is not required now by TS/JS but it might be needed at some point
  • multiple patterns. Can I provide multiple ones (give me all js/ts, tsconfigs and package.json files) when searching for a files? I think that sometimes performance of single request with multiple patterns may be better than performance of multiple requests with single pattern each

WDYT?

@felixfbecker
Member
felixfbecker commented Nov 2, 2016 edited

@alexsaveliev multiple patterns is a good point, I would change the pattern param to string | string[]. Then you can prefix globs with ! to exclude files. .git could also be excluded by the dot: false option, do you need that? What do you think about #4?

@felixfbecker
Member
felixfbecker commented Nov 2, 2016 edited

@slimsag What are the use cases that you have in the go langserver atm? The problem I see with a fs/* namespace is that is coupled to the presence of a file system, but a client might not even work on a file system, but over an arbitrary protocol that might not support stat operations etc. So I think it is best to reduce the protocol to the minimum required to make the LS work and to something that clients can implement over almost any protocol (globbing, getting the content). wdyt?

@sqs
Member
sqs commented Nov 7, 2016

Closing in favor of #4

@sqs sqs closed this Nov 7, 2016
@sqs sqs deleted the extension-fs branch Nov 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment