Support incremental document updates #1141

david-driscoll · 2019-03-08T03:59:36Z

Hi All,

I've recently run into a slow performance issue like #1126, and figured I would try an do something about it.

After doing some debugging and live testing, I determined that the issue is diagnostics are taking to long process. At that point I noticed the comment on IScriptSnapshot...

        /**
         * Gets the TextChangeRange that describe how the text changed between this text and
         * an older version.  This information is used by the incremental parser to determine
         * what sections of the script need to be re-parsed.  'undefined' can be returned if the
         * change range cannot be determined.  However, in that case, incremental parsing will
         * not happen and the entire document will be re - parsed.
         */
        getChangeRange(oldSnapshot: IScriptSnapshot): TextChangeRange | undefined;

My hypothesis is that all documents are being parsed and diagnostics are being recalculated for the entire process.

This changes does the following...

Adds support for incremental updates over lsp
Uses text edits to make incremental changes to documents
Adds a new DocumentInfo class that contains the parsed regions for a given vue document, which are only recomputed on update.

With the new DocumentInfo class we can properly create a IScriptSnapshot for vue related files, which greatly improves the time it takes for diagnostics to be computed.

Things I'm still working on / would love help with.

Unit Tests
I seem to have broken something around stylus, not sure why.
e2e tests
The behaviors I'm seeing locally everything is working "normally" (this doesn't mean it's correct, it just appears to be working for me).

I have just recently got these changes working locally, so over the next few days I'll be using my updated server on some projects I'm working on to validate the changes.

Also supports incremental compliation for JavaScript / TypeScript

… with regions not working. still have some integration test failures

…mpilation as existing setup was not working correctly

david-driscoll · 2019-03-09T04:47:06Z

I think I've squared away most of the outstanding issues and I've gotten the integration tests passing successfully. Please feel free to let me know what other changes need to be made (commit messages, etc).

Thanks!

octref · 2019-03-20T20:00:26Z

Wow, thank you so much for the contribution. I'm just starting to look into this.
Looks mostly fine. Let me upgrade to TS 3.3 and get back to you.

Meanwhile can you allow edits from maintainers?

david-driscoll · 2019-04-01T20:47:12Z

@octref I've checked off the allow maintainers to commit checkbox.

I'm also going to update to the latest master as well.

octref · 2019-04-02T22:16:13Z

client/client.ts

@@ -29,11 +29,74 @@ export function initializeLanguageClient(vlsModulePath: string): LanguageClient
    debug: { module: serverPath, transport: TransportKind.ipc, options: debugOptions }
  };

+  const watcher = (() => {
+    const innerWatcher = vscode.workspace.createFileSystemWatcher('{**/*.js,**/*.ts}', true, false, true);


Listen to file creation as well, see #1091.

octref · 2019-04-02T22:17:49Z

package-lock.json

@@ -0,0 +1,4434 @@
+{


Please use yarn instead of npm

octref · 2019-04-02T22:17:57Z

server/package-lock.json

@@ -0,0 +1,7071 @@
+{


Please use yarn instead of npm

octref · 2019-04-03T03:48:19Z

test/lsp/references/basic.test.ts

@@ -57,7 +57,7 @@ async function testReferences(docUri: vscode.Uri, position: vscode.Position, exp
  expectedLocations.forEach(el => {
    assert.ok(
      result.some(l => {
-        return l.range.isEqual(el.range) && l.uri.path === el.uri.path;
+        return l.range.isEqual(el.range) && l.uri.path.toLowerCase() === el.uri.path.toLowerCase();


Is not changing this failing some tests?

While doing https://github.com/vuejs/vetur/pull/907/files#diff-3da884033e59974c36d7c1a57a067d8dR221 I realized serviceHost default to being case insensitive. I fixed that so you probably don't need this any more.

That caused some weird behaviors for me like #1170 (comment), when I upgraded to 3.3.

octref · 2019-04-03T03:48:40Z

test/lsp/diagnostics/helper.ts

-  const result = vscode.languages.getDiagnostics(docUri);
+  let result: vscode.Diagnostic[] = [];
+  while (result.length === 0) {
+    await sleep(1000);


Thanks, I totally should have done that to avoid flakey diagnostic tests...

octref · 2019-04-03T03:50:05Z

test/lsp/definition/basic.test.ts

@@ -42,5 +42,5 @@ async function testDefinition(docUri: vscode.Uri, position: vscode.Position, exp
  )) as vscode.Location[];

  assert.ok(result[0].range.isEqual(expectedLocation.range));
-  assert.equal(result[0].uri.path, expectedLocation.uri.path);
+  assert.equal(result[0].uri.path.toLowerCase(), expectedLocation.uri.path.toLowerCase());


See the reference test comment.

I was again getting weird casing issues on my computer (windows) where the test was failing even the path was valid. in one case the path was c:/.../ vs C:/.../

octref · 2019-04-03T03:50:38Z

test/lsp/completion/script.test.ts

@@ -10,7 +10,7 @@ describe('Should autocomplete for <script>', () => {
    await showFile(scriptDocUri);
    await sleep(FILE_LOAD_SLEEP_TIME);
    // TS LS completion starts slow.
-    await sleep(2000);
+    await sleep(5000);


Not sure if this is the desperate change to fix the Stylus issue. If it's failing just leave it for now.

octref · 2019-04-03T03:50:46Z

test/lsp/completion/helper.ts

-              (match.documentation as vscode.MarkdownString).value,
-              (ei.documentation as MarkupContent).value
+              (match.documentation as vscode.MarkdownString).value.replace(/\r/g, ''),
+              (ei.documentation as MarkupContent).value.replace(/\r/g, '\n')


Why this change?

I was running into issues locally (I'm on windows) where the carriage return was causing tests to fail.

octref · 2019-04-03T04:01:13Z

server/src/modes/script/serviceHost.ts

+
+  constructor(documentRegion: DocumentRegion) {
+    this.textSnapshot = documentRegion.document.getText() || '';
+    this.version = `${documentRegion.document.version}:${documentRegion.editRanges.length}`;


Why editRanges.length?

octref · 2019-04-03T04:24:28Z

Looks very promising. Impressive work!

Adds a new DocumentInfo class that contains the parsed regions for a given vue document, which are only recomputed on update.

I'm wondering if you thought about matching textChange against region boundaries to only re-parse the region that has changed. It'll probably be another set of huge changes. We can leave that for another PR.

I seem to have broken something around stylus, not sure why.

Don't worry about that for now. I can take a look later.

The behaviors I'm seeing locally everything is working "normally" (this doesn't mean it's correct, it just appears to be working for me).

Travis sometimes are flakey. But I'd say don't skip the test suites. Let them fail with messages so i can dig into them as well.

IScriptSnapshot

Correct me if I'm wrong, but here's my understanding:

TS asks from ServiceHost a IScriptSnapshot when it needs the updated file
In the old model, we trigger diagnostics on each file change, which triggers TS to re-compute diagnostics on each file change.

If my understanding are correct, is debouncing on diagnostics (not file updates, just the diagnostic) likely to improve perf?

And for the part where you try to compute the diff between snapshots, I'd suggest keeping an ordered list of (textDocument, version, edits), and when asked for snapshot between (m, n), use the edits between (m, n) to figure out the ts.TextChangeRange and then drop every item before n.

Let me check with my friends in TS team to see how they handle the snapshots exactly...

david-driscoll · 2019-04-10T15:06:53Z

I definitely need to fix up the snapshots a bit. I had originally had computing the text change range in place, but ran into inconsistency issues, so I dropped that for the moment. As I'm still seeing some performance issues depending on the vue file I'm running in, I'm probably going to try and add it back in and see what happens.

I'll try and work on fixing the conflicts today if I can, and then look at the ranges again.

octref · 2019-04-10T15:49:46Z

Thanks, no hurries, I can try to resolve a few conflicts from master as well, since I made most of those changes...

david-driscoll · 2019-04-17T23:20:11Z

@octref I've merged all the changes against master now, and gotten everything working on my side.

Let me know what changes need to be made and I'll make them.

After this change is in I'm going to work on support for the vue-class-component and vue-property-decorator for intellisense as well.

PS: Just playing with the new template compiler support, and it's pretty awesome!

david-driscoll · 2019-04-18T01:17:44Z

I'm currently working on the test failures locally, but I have a different set of failures than the failures in travis... yay!

david-driscoll · 2019-04-18T01:19:15Z

Any feedback you might have to help fix them would be great as well.

octref · 2019-04-26T04:09:08Z

Sorry for my lack of response, busy on other things. I'll make sure to get back to you tomorrow.

david-driscoll · 2019-04-30T13:36:08Z

@octref no worries, I totally get being busy with other things! :)

garyo · 2019-10-19T23:23:00Z

Hi - I'm just wondering whether this PR might see the light of day sometime soon -- I'm seeing very slow performance with VLS and emacs lsp-mode and I'm hoping this would improve it.

david-driscoll added 3 commits March 7, 2019 22:33

Initial work on support incremental updates

84d4bf2

Also supports incremental compliation for JavaScript / TypeScript

Merge branch 'master' of github.com:vuejs/vetur

11b4bbe

Fixed linting error

9d321fe

david-driscoll marked this pull request as ready for review March 8, 2019 03:59

david-driscoll added 7 commits March 8, 2019 11:25

flow document info through where textdocument used to be. Fixed issue…

396f829

… with regions not working. still have some integration test failures

make DocumentInfo a TextDocument; Simplified logic for incremental co…

fe9d7b8

…mpilation as existing setup was not working correctly

Updated

5ba9c7d

fixed some of the tests to avoid issues when running on windows

3300b0e

Updated service host to create the underlying documents correctly

f05ca8f

fixed diagnostic tests

432084a

clean up some commented out code

cb7f4b7

david-driscoll changed the title ~~WIP: Incremental updates~~ Support incremental document updates Mar 9, 2019

octref mentioned this pull request Mar 21, 2019

Upgrade to TS 3.3 #1163

Closed

david-driscoll added 2 commits April 1, 2019 16:21

Updated to better deal with external documents

04cae09

simplified external documents removing content

7cd9643

david-driscoll added 4 commits April 1, 2019 17:22

Merge remote-tracking branch 'origin/master' into incremental-updates

2f7cc02

Fixed remaining compiler errors

dc94ed5

fix last compiler issues

618f6e0

Merge remote-tracking branch 'origin/master' into incremental-updates

4bee21b

octref reviewed Apr 3, 2019

View reviewed changes

octref mentioned this pull request Apr 11, 2019

High CPU Usage #1051

Closed

3 tasks

david-driscoll added 3 commits April 17, 2019 18:04

improved performance by caching snapshots with documents

d595b7b

Re-renable skipped tests

483eb97

Merge remote-tracking branch 'origin/master' into incremental-updates

11d3dc5

Fixed changes to update to latest from origin

476a296

rollback some test changes

fa0d8f2

octref mentioned this pull request May 1, 2019

Performance meta issue #1265

Open

9 tasks

minor change

1e88c79

lanyizi mentioned this pull request Aug 29, 2020

Sometimes Vetur is still very slow #2192

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support incremental document updates #1141

Support incremental document updates #1141

david-driscoll commented Mar 8, 2019

david-driscoll commented Mar 9, 2019

octref commented Mar 20, 2019

david-driscoll commented Apr 1, 2019

octref Apr 2, 2019

octref Apr 2, 2019

octref Apr 2, 2019

octref Apr 3, 2019

octref Apr 3, 2019

octref Apr 3, 2019

octref Apr 3, 2019

david-driscoll Apr 18, 2019

octref Apr 3, 2019

octref Apr 3, 2019

david-driscoll Apr 18, 2019

octref Apr 3, 2019

octref commented Apr 3, 2019

david-driscoll commented Apr 10, 2019

octref commented Apr 10, 2019

david-driscoll commented Apr 17, 2019

david-driscoll commented Apr 18, 2019

david-driscoll commented Apr 18, 2019

octref commented Apr 26, 2019

david-driscoll commented Apr 30, 2019

garyo commented Oct 19, 2019

Support incremental document updates #1141

Are you sure you want to change the base?

Support incremental document updates #1141

Conversation

david-driscoll commented Mar 8, 2019

david-driscoll commented Mar 9, 2019

octref commented Mar 20, 2019

david-driscoll commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octref commented Apr 3, 2019

david-driscoll commented Apr 10, 2019

octref commented Apr 10, 2019

david-driscoll commented Apr 17, 2019

david-driscoll commented Apr 18, 2019

david-driscoll commented Apr 18, 2019

octref commented Apr 26, 2019

david-driscoll commented Apr 30, 2019

garyo commented Oct 19, 2019