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

Semantic colorization requested for entire (large) file instead of viewport area #92963

Closed
RyanCavanaugh opened this issue Mar 18, 2020 · 3 comments
Assignees
Labels
freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues semantic-tokens Semantic tokens issues typescript Typescript support issues

Comments

@RyanCavanaugh
Copy link
Member

  • VSCode Version: 1.43 (UX regression from 1.42)
  • OS Version: Windows 10

Steps to Reproduce:

  1. Open Microsoft/typescript src/compiler/checker.ts (~36,000 lines)
  2. Request completion somewhere in the file after a little warm-up
  3. Observe delay: ~8 seconds on a middle-of-the-road machine
  4. Turn off semantic highlighting
  5. Try again: Delay is more like ~3 seconds

The root cause seems to be VS Code sending repeated commands on edit for

Info 250  [10:51:9.601] request:
    {"seq":27,"type":"request","command":"encodedSemanticClassifications-full","arguments":{"file":"d:/github/TypeScript/src/compiler/checker.ts","start":0,"length":2188700}}

specifically

"start":0,"length":2188700 <- too much!

which in turn causes TS Server to perform a ton of work highlighting nonvisible parts of the file

Does this issue occur when all extensions are disabled?: Yes

It seems like this was being discussed at #86415 but raising this for clarity of impact

@alexdima
Copy link
Member

Two things:

  1. The document range semantic tokens provider is not yet invoked by vscode. The plan is that TS will register a document range semantic tokens provider, as well as a document semantic tokens provider. So for the initial open of checker.ts, first the semantic tokens in the viewport will be fetched, and then the entire document will be fetched. This will make the initial coloring come faster for the viewport. But still on each edit, the document semantic tokens will be fetched. This is tracked in Implement range semantic tokens provider #92273

  2. TS does not implement any delta strategy when talking to tsserver to fetch semantic tokens, so the extension host always fetches the entire document and tsserver computes the semantic tokens for the entire document and returns all of them on the wire. In our opinion, this is not optimal, and that is why we have a delta mechanism where changes to semantic tokens can be returned by the extension host. The TS extension does not make use of this mechanism. This mechanism is implemented as a proposal in the LSP, but to implement it for typescript, this needs to be implemented in the tsserver protocol, and cannot be implemented via a ts server plugin that decorates replies.

Given 1. is tracked in #92273 , I suggest we keep this issue around for 2. I don't know who will own this. The initial implementation (using a ts server plugin) has been done by @aeschli but maybe this now is owned by @mjbvz , I don't know.

@alexdima alexdima assigned aeschli and mjbvz and unassigned alexdima Mar 19, 2020
@alexdima alexdima added semantic-tokens Semantic tokens issues typescript Typescript support issues freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues labels Mar 19, 2020
@RyanCavanaugh
Copy link
Member Author

@alexdima Thanks for the info. In our opinion, semantic highlighting should be off by default (at least in JS / TS) until it can be deployed in a way that doesn't incur exceptional delays in large files - this is quickly going to create UX problems for us.

cc @mjbvz

@mjbvz mjbvz removed their assignment Mar 20, 2020
@aeschli
Copy link
Contributor

aeschli commented Mar 29, 2020

#92789 is the issue to facilitate the ownership transfer for the TS semantic highlighting from us to the TypeScript team. With that goes the request to improve or replace the encodedSemanticClassifications-full request to support deltas.

I will annotated #92789 and close this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues semantic-tokens Semantic tokens issues typescript Typescript support issues
Projects
None yet
Development

No branches or pull requests

4 participants