Improved performance of addChange() #4380
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Travis tests have failedHey @CVVPK, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; finpm run buildnpm testTravisBuddy Request Identifier: 390ad000-1e23-11ea-a424-4f8e788bb22e |
J-Fields
reviewed
Dec 14, 2019
|
|
||
| if (newText === this.oldText) { | ||
| // Check if the document's text changed. | ||
| if (newTextLength === this.oldText.length) { |
There was a problem hiding this comment.
Perhaps we can just enumerate the special cases and deal with them separately, but doesn't this break with :s/abc/def, for instance?
There was a problem hiding this comment.
I believe you're referring to the case of doing iabc<Esc>:s/abc/def\nu which results in | when it should be abc. It does break that, sorry. I hadn't considered that case and it seems we aren't currently testing for it. Can I add a test for it?
There was a problem hiding this comment.
Yeah a test for this would be great. If we go in this direction of optimization, I'd like to make sure we cover all the edge cases
|
I'm closing this because I messed up my master.... |
J-Fields
pushed a commit
that referenced
this pull request
Jan 9, 2020
For reference, see #4380 and #3920 . As mentioned in #4380 HistoryTracker.addChange() performance gets worse the larger the file and the faster we make individual changes, because on each change we would get the entire document's text. We don't use the individual changes, so we really only care about the initial step and the final one. A new historyStep is created when we first enter a new mode and finalized after we exit it. We can keep track of the currentMode internally, and when it is different to vimState.currentMode we know we have switched. This way we prevent making unnecessary calls to TextDocument.getText() while changes are still in progress. This commit also activates the optimization from #4451 when `experimentalOptimizations` is enabled.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, HistoryTracker.addChange() has a major impact on performance, specially when working on large files. The issue stems largely due to processing the entire document on every call, and that the function gets called on every key press. My proposed changes are as follows:
Check if offset at EOF is equals to oldText.length
Currently the first thing addChange() does is grab the text of the entire document, which is a pretty expensive operation. We then check the newText against the oldText to see if there are any changes that need to be processed. There really is no need to get the entire document to know this.
The zero based offset at the document's end represents the location of the character that would follow the last character, thus it is equal to the length of the document's text. We can easily get this offset by using vscode.TextDocument.offsetAt() and passing it Position.getDocumentEnd(). If there are no changes this offset will be equals to historyTracker.oldText.length.
This would increase the performance when navigating through large files.
Improvements while on insert mode
The major issue is that addChange() gets called on every key press, thus the entire document is getting processed every few milliseconds if we type too fast. Which is why we experience more lag the faster we make changes on large files. Currently the first change after going into insert mode triggers _addnewHistoryStep(), consequent changes are added to the historyStep and then when we get out of insert mode we finalise that historyStep.
We shouldn't really need to log each substep while we are on Insert Mode since we only go back one historyStep at a time. We can grab a snapshot of when we first start making changes and one at the end. I had initially thought we could just grab the last change, but we do need the first step to be logged so that we don't brake historyTracker.goBackHistoryStepsOnLine().
We can add a condition that checks if the currentMode === Mode.Insert to get out of addChange() and prevent processing the document while changes are still in progress. The condition shouldn't trigger if we just created a new history step, so that we may log the first step. When we get out of Insert Mode, the call from to addChange() within ModeHandler.runAction() will process the final step.
This change, while it has a major positive impact on performance, does change the workflow of the code a little bit, since calls to addChange() while we are on insert mode will not do anything. With the exception of the first change that creates the historyStep, the call made to addChange() from within ModeHandler.handleKeyEventHelper() will be go largely ignored. I don't believe this has any negative side effects, but is something we might want to be aware of.
Which issue(s) this PR fixes
Possibly fixes #3920, #2021, #2216