Integrate Piece Tree -> IncrementalTextDocument | TextDocuments Incremental Document Support#478

Jma353 · 2019-04-09T23:05:43Z

This pull request is the result of a dialogue in #476.

This PR does 2 things:

Integrates @rebornix's vscode-piece-tree npm module. I use this module to implement IncrementalTextDocument, which is the Incremental sync document representation (as FullTextDocument is the Full sync document representation). This is a class internal to server module.
Updates the TextDocuments document manager to support Incremental text synchronization mode. This manager defaults to Full, but has an optional constructor parameter that allows the consumer of the API to specify Incremental document sync. When Incremental document sync is specified, this document manager passes isFullSync = false to TextDocument#create, which results in IncrementalTextDocument being used per document.

This PR also bootstraps a test suite for TextDocuments (specifically involving onChange events) + a test suite for IncrementalTextDocument via TextDocuments document manager.

Output of server tests:

  Connection Tests
    ✓ Ensure request parameter passing
    ✓ Ensure notification parameter passing

  TextDocuments Tests
    ✓ onDidChangeContent TextDocumentSyncKind.Full change file content
    ✓ onDidChangeContent TextDocumentSyncKind.Full several content updates
    ✓ onDidChangeContent TextDocumentSyncKind.Incremental removing content
    ✓ onDidChangeContent TextDocumentSyncKind.Incremental adding content
    ✓ onDidChangeContent TextDocumentSyncKind.Incremental replacing content
    ✓ onDidChangeContent TextDocumentSyncKind.Incremental several content changes

  IncrementalTextDocument
    ✓ Single line document
    ✓ Multiple line document
    ✓ Varying newline characters
    ✓ getText(Range)
    ✓ Invalid inputs
    ✓ Basic append
    ✓ Multi-line append
    ✓ Basic delete
    ✓ Multi-line delete
    ✓ Single character replace
    ✓ Multi-character replace
    ✓ Invalid update ranges


  20 passing (37ms)

msftclas · 2019-04-09T23:05:54Z

All CLA requirements met.

rebornix · 2019-04-10T00:08:25Z

server/src/main.ts

+					let updatedDoc: UpdateableDocument = edits.reduce(
+						(workingTextDoc: UpdateableDocument, edit) => {
+							let newText = TextDocument.applyEdits(workingTextDoc, [edit]);
+							workingTextDoc.update({ text: newText }, version);


Currently every update on the TextDocument will flush the _lineOffset

https://github.com/Microsoft/vscode-languageserver-node/blob/9de8809400b7c443bc25f211dfaddcb74675b2d7/types/src/main.ts#L2654

while applyEdits relies on _lineOffset https://github.com/Microsoft/vscode-languageserver-node/blob/9de8809400b7c443bc25f211dfaddcb74675b2d7/types/src/main.ts#L2500-L2501

It means for every content change we try to apply, it will go through the whole document, character by character, to find all line breaks. In real world scenarios it can be really costly.

Yup, I definitely observed this limitation.

These black-box functions (applyEdits / update) are the only ways to currently update text documents. As we discussed in #476, I can use your npm module in this PR and swap out the implementations for these core functions as well, so that these calls don't rescan the entire document.

Apologies, when I originally wrote this up, I wasn't aware that you had already published the package to npm.

No worries and you are doing a great job here. I'm just a bit surprised to see that lineOffsets were being calculated every time when we do full sync (the good part is we calculate only when reading line info).

Based on your proposal and this PR, I think we can see two major performance improvement

We don't need to send full document from client to server anymore when the sync mode is incremental

The line offset calculation is incremental when the sync mode is incremental, which is actually the major time spent in loading a file in VS Code.

Let me know if you need any help from me, I can push directly to your PR with piece tree related changes (if you are okay with that).

If you've made some sort of local progress on adding your piece tree as a dependency and using it, then feel free to push it to this PR. Otherwise, I was planning on adding it as a dependency + using it in FullTextDocument / TextDocument namespace today.

Feel free to let me know, and we can proceed from there.

@Jma353 go for it, I don't have anything yet, look forward to your change.

For sure - I'll update this today or tomorrow.

Jma353 · 2019-04-16T00:05:13Z

types/src/main.ts

+
+		// Prepare Piece Tree
+		const ptBuilder = new PieceTreeTextBufferBuilder();
+		ptBuilder.acceptChunk(content);


@rebornix - should I be slicing this up into 64 KB chunks, as you mentioned is done by fs.readFile in your blog post, or is this an appropriate use of your API (passing the entire content "chunk" in on instantiation).

We don't need to. If we do simple slice, then there is no difference as SlicedString in v8 is just a mapping to part of the original string. If we create copies from that, the initialization will be costly.

It would be great that if the document data we get from client comes as chunks instead of a single string, and then we can benefit from that.

… TextDocument#create docstring

Jma353 · 2019-04-16T00:45:57Z

@dbaeumer given that I add piece tree npm module in this PR, unsure if I now need to add a thirdpartynotices.txt file in types, similar to the one in client. I'm assuming vscode-piece-tree will eventually become first-party (under Microsoft github).

rebornix · 2019-04-16T17:58:43Z

types/src/main.ts

@@ -2611,6 +2616,15 @@ export interface TextDocumentContentChangeEvent {
 	text: string;


Should we update the comments for text? In FullSync mode, text is always the content of the full document from client side but now in incremntal mode, it's the change for the corresponding range. Another thing we need to make sure that Client sends only content change but not full text in incremtnal mode.

https://github.com/Microsoft/vscode-languageserver-node/blob/053149a76e7ec147347e41a5847a1cc4ccb3d541/client/src/client.ts#L936-L940

It's already guaranteed that when it's incremental, it sends the change event but not the full doc. So we only need to update the documentation.

Will update the documentation in the next commit to the PR :)

types/src/main.ts

Jma353 · 2019-04-18T18:44:17Z

Outside of updating documentation, any additional feedback on this? @rebornix, @dbaeumer, @jrieken? I can address it all in one commit if there's any more.

rebornix · 2019-04-26T16:38:41Z

@Jma353 the code looks good to me and thanks for your help! I'll love to hear feedback from others as well, considering some of them are in vacation, I think you can push changes and even better do some benchmark.

Jma353 · 2019-05-03T17:59:18Z

@rebornix I'll submit doc change + will setup a lightweight benchmark this weekend.

In general, I feel as though an explicit win of this is just the idea that we can support incremental sync-ing in the first place. Sending full files between processes isn't cheap.

dbaeumer · 2019-05-08T12:08:48Z

types/src/main.ts

 	 */
-	export function create(uri: string, languageId: string, version: number, content: string): TextDocument {
-		return new FullTextDocument(uri, languageId, version, content);
+	export function create(uri: string, languageId: string, version: number, content: string, isFullSync: boolean = true): TextDocument {


Can we make isFullSync a enum or string or type. It more feels like a enum in case we have another sync expierence later on.

The boolean looks strange, and the return type is always TextDocument. It's unclear to an API user when to set isFullSync and when not.
What about defining a separate constructor?

When doing that I would also officially introduce the UpdatableTextDocument. The default implementation of a UpdatableTextDocument should be the new piece tree document as it is optimised for updates.

export interface UpdatableTextDocument extends TextDocument { update(event: TextDocumentContentChangeEvent, version: number) } export namespace UpdatableTextDocument { export function create(uri: string, languageId: string, version: number, content: string) : UpdatableTextDocument { return new IncrementalTextDocument(uri, languageId, version, content); } }

dbaeumer · 2019-05-08T12:10:20Z

Other than the above comment things look good to me.

@aeschli do you have any input / comments?

aeschli · 2019-05-09T06:37:17Z

My comment is about TextDocument.create(uri: string, languageId: string, version: number, content: string, isFullSync: boolean = true): TextDocument

The isFullSync looks strange, and the return type is always TextDocument. It's unclear to an API user when to set isFullSync and when not.
What about defining a separate constructor?

When doing that I would also officially introduce the UpdatableTextDocument. The default implementation of a UpdatableTextDocument should be the new piece tree document as it is optimised for updates.

export interface UpdatableTextDocument extends TextDocument {
    update(event: TextDocumentContentChangeEvent, version: number)
}
export namespace UpdatableTextDocument {
    export function create(uri: string, languageId: string, version: number, content: string) : UpdatableTextDocument {
       return new IncrementalTextDocument(uri, languageId, version, content);
   }
}

TextDocument.create should continue to return a FullTextDocument for now, and we should also keep FullTextDocument.update, but we should fix it to also work correctly with a range.
At some point we could decide to remove the FullTextDocument and only use the new IncrementalTextDocument also for TextDocument.create.

dbaeumer · 2019-05-24T13:53:29Z

I though about this again and I am in favour of making this more explicit. However I don't like the name UpdatableTextDocument since FullTextDocument are updateable as well. To make this in line with the TextDocumentSyncKind I would call them FullTextDocument and IncrementalTextDocument. We should deprecate the TextDocument namespace and expose a FullTextDocument interface and namespace. The FullTextDocument interface would define an update(text: string, version) method. The IncrementalTextDocument text document a update(event: TextDocumentContentChangeEvent, version: number) method. We can also add is methods to the namespace which do an instanceof against the private class.

@Jma353 are you willing to do these changes?

rebornix · 2019-05-24T17:11:27Z

I prefer FullTextDocument and IncrementalTextDocument as well, which aligns with TextDocumentSyncKind.

Jma353 · 2019-05-29T18:40:22Z

These changes seem reasonable. I appreciate the feedback all!

Apologies for the slowness in my response (been busy with some other things). I will prioritize iterating on this by EOW.

aeschli · 2019-05-29T19:40:26Z

@Jma353 @rebornix Me and Dirk had some more discussion and came up with one more suggestion, which has the smallest impact on the API:

we keep types as-is, no new APIs
IncrementalTextDocument stays in protocol as an internal class to be used byTextDocuments and optimized for its needs.

Sorry for the back and forth. If you are very busy then I'm happy to merge and make the changes. Just let me know.

Jma353 · 2019-05-29T20:15:52Z

I can change these things @aeschli - thanks for the thoughtful feedback.

Expect an update with said changes by Friday, this way you don't have to do any additional refinements on your part.

Jma353 · 2019-05-31T17:26:17Z

@aeschli did you mean server (not protocol) in your above feedback?

protocol seems to be only namespace and interface declarations. Additionally, TextDocuments is in server subdirectory.

I can remove changes from types directory. The only API change that will result from this PR is an optional argument passed to TextDocuments ctor. I currently pass it as a enum value.

Jma353 · 2019-05-31T21:23:02Z

I have updated the PR according to feedback, to limit API changes. Here is a summary:

As mentioned, the only API change is an optional constructor parameter passed to TextDocuments, which allows for enum-based switching between FullTextDocument and IncrementalTextDocument.
IncrementalTextDocument is now a class internal to server (since it's only used by TextDocuments).
I have ported all test-cases that existed in types for IncrementalTextDocument to server. I have the suite in the TextDocuments test file.
I have rebased on @rebornix 's change to the piece tree npm version, as well as latest master.

rebornix · 2019-05-31T21:27:13Z

Considering @Jma353 's great effort is going to be merged into the project, I'll add some scripts to VS Code in June to ship piece tree directly from there instead of having my own repository, similar to how we ship Monaco.

Jma353 · 2019-05-31T21:29:55Z

@rebornix as in have piece tree available via vscode API suite?

A.k.a. import {PieceTreeBase, ...} from 'vscode';?

…test case

rebornix · 2019-06-03T18:53:01Z

@Jma353 I didn't go that route finally but aligned with how we ship vscode-uri, which has its implementation in its own repo. I published a npm package called vscode-textbuffer, the api doesn't change at all and the repo will be under microsoft (will be public soon).

update

vscode-textbuffer is public now https://github.com/microsoft/vscode-textbuffer

dbaeumer · 2019-06-04T09:46:52Z

@Jma353 I am correct that you still need to adopt the new vscode-textbuffer npm package. Right?

rebornix · 2019-06-04T18:17:55Z

I replaced vscode-piece-tree with vscode-textbuffer and comments related to that. @dbaeumer is it good to go?

Jma353 · 2019-06-04T21:55:17Z

@rebornix I see travis failure relating to the new module.

Thanks for swapping out npm modules btw :)

rebornix · 2019-06-04T23:26:23Z

@Jma353 thanks it's green green now.

dbaeumer · 2019-06-05T10:18:37Z

@Jma353 & @rebornix thanks a lot for making this happen.

TextDocuments incremental document sync plus tests

b05bde0

Jma353 mentioned this pull request Apr 9, 2019

Leverage vscode "Piece Tree" string buffer in TextDocument representation #476

Closed

rebornix reviewed Apr 10, 2019

View reviewed changes

Jma353 added 2 commits April 15, 2019 16:54

Add piece tree dependency, implement IncrementalTextDocument, and test

595fd94

Remove console log try-catch from debugging

8a1ce5d

Jma353 commented Apr 16, 2019

View reviewed changes

Jma353 added 2 commits April 15, 2019 17:07

Fix typo in comment

8edc6a7

Rename boxed -> clamped, move comment

7fa6603

Jma353 changed the title ~~TextDocuments Incremental Document Sync Support + Tests~~ Integrate Piece Tree -> IncrementalTextDocument | TextDocuments Incremental Document Support Apr 16, 2019

Add edge-case update tests (before + after document range) and update…

92a5e9c

… TextDocument#create docstring

rebornix reviewed Apr 16, 2019

View reviewed changes

types/src/main.ts Outdated Show resolved Hide resolved

Update piecetree

47a5883

dbaeumer reviewed May 8, 2019

View reviewed changes

dbaeumer added this to the May 2019 milestone May 9, 2019

Jma353 added 2 commits May 31, 2019 13:59

Clean up types, setup incremental doc sync in server

ca741bd

Merge branch 'master' into server/incremental-text-documents-sync

7acb1ff

Jma353 added 3 commits May 31, 2019 14:08

Fix areas of code affected by merge with master

d4ce770

fix typos

cb178f5

Remove modifications of types/src/main.ts

fcd5bfa

Remove IncrementalTextDocument label from incremental doc test suite …

f1c8e8e

…test case

dbaeumer modified the milestones: May 2019, June 2019 Jun 4, 2019

Update vscode-textbuffer

ded1041

update vscode-textbuffer output folder

d9b5d7b

dbaeumer merged commit f14d94b into microsoft:master Jun 5, 2019

lukesmurray mentioned this pull request Sep 22, 2020

editor lag in large projects kortina/vscode-markdown-notes#90

Open

		@@ -2611,6 +2616,15 @@ export interface TextDocumentContentChangeEvent {
		text: string;

Conversation

Jma353 commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jma353 Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rebornix Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jma353 commented Apr 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jma353 commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebornix commented Apr 26, 2019

Uh oh!

Jma353 commented May 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbaeumer commented May 8, 2019

Uh oh!

aeschli commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbaeumer commented May 24, 2019

Uh oh!

rebornix commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jma353 commented May 29, 2019

Uh oh!

aeschli commented May 29, 2019

Uh oh!

Jma353 commented May 29, 2019

Uh oh!

Jma353 commented May 31, 2019

Uh oh!

Jma353 commented May 31, 2019

Uh oh!

rebornix commented May 31, 2019

Uh oh!

Jma353 commented May 31, 2019

Uh oh!

rebornix commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbaeumer commented Jun 4, 2019

Uh oh!

rebornix commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jma353 commented Jun 4, 2019

Jma353 commented Apr 9, 2019 •

edited

Loading

msftclas commented Apr 9, 2019 •

edited

Loading

Jma353 Apr 10, 2019 •

edited

Loading

rebornix Apr 16, 2019 •

edited

Loading

Jma353 commented Apr 18, 2019 •

edited

Loading

aeschli commented May 9, 2019 •

edited

Loading

rebornix commented May 24, 2019 •

edited

Loading

rebornix commented Jun 3, 2019 •

edited

Loading

rebornix commented Jun 4, 2019 •

edited

Loading