Improve performance on large files by not checking for changes after commands which never change the document #4451
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
J-Fields
force-pushed
the
mightChangeDocument
branch
from
c254b21 to
25ecda3
Compare
Travis tests have failedHey @J-Fields, 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 testTravisBuddy Request Identifier: fa7af9a0-2c5f-11ea-a6f9-ed85cdbf407f |
J-Fields
force-pushed
the
mightChangeDocument
branch
from
25ecda3 to
6fbb574
Compare
Travis tests have failedHey @J-Fields, 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 testTravisBuddy Request Identifier: f15dd940-2c65-11ea-a6f9-ed85cdbf407f |
…commands which never change the document Many commands, like movement and typing a search, will never change the document. We waste considerable time diffing the entire document on each of these keystrokes to figure out if we should add an undo step. I've added a function to BaseAction, `mightChangeDocument`, which determines whether we should check for changes. This defaults to false for the sake of brevity (most commands actually don't change the document), but this is a bit dangerous - what if I forgot to override it in a command that changes the document? The planned solution is to release this as a check - we will always run the diff, as we currently do, then make sure a command didn't add a change when we asserted it wouldn't. After this is in the wild for a little while with no reported issues, we'll actually flip the switch to optimize. In my testing on a ~80,000 line file, this saves maybe 10-15ms per keystroke on average, which does translate into a noticeable improvement in responsiveness.
J-Fields
force-pushed
the
mightChangeDocument
branch
from
6fbb574 to
abc2afd
Compare
cvaldev
reviewed
Jan 1, 2020
| ? recordedState.operator.mightChangeDocument | ||
| : action.mightChangeDocument; | ||
| if (addedChange && !mightChangeDocument) { | ||
| throw new Error(`Action '${action.toString()}' changed the document unexpectedly!`); |
There was a problem hiding this comment.
This will always throw with #4386 when switching out of Insert mode using <C-c> and <C-o> because _isDocumentTextNeeded() is expecting modes to be different so that the state of the doc after the last change is recorded. When this addChange() gets called we will be in Normal Mode and thus <C-c> (key=<copy>) might not change the document, which is correct. Typically we would have recorded the changes just before switching out of Insert mode because the addChange() in ModeHandler.handleKeyEventHelper() takes care of that before the action is ran.
There was a problem hiding this comment.
Maybe have a action.willSwitchModes and pass that over to addChange() in HistoryTracker.handleKeyEventHelper?
There was a problem hiding this comment.
I'm tempted to just set mightChangeDocument to true on these two commands and add to the mightChangeDocument comment explaining that since we only want to diff at the end of insert mode, a command that goes from insert mode to normal mode does (sort of) change the document. Thoughts on that?
There was a problem hiding this comment.
I think <C-o> (CommandOneNormalCommandInInsertMode) should definitely have mightChangeDocument = true since that's exiting Insert mode. <C-c> (CommandEscInsertMode) already has it properly set, but I believe is failing because it gets overwritten to <copy>.
There was a problem hiding this comment.
I guess setting mightChangeDocument = true on <copy> would solve it.
There was a problem hiding this comment.
Yeah I like this idea better, would you like me to just commit these changes into #4386?
J-Fields
added a commit
that referenced
this pull request
Jan 4, 2020
J-Fields
added a commit
to cvaldev/Vim
that referenced
this pull request
Jan 4, 2020
…ions` is enabled
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.
|
Hi, I activated this feature to try and test it. But I think, I found some issues in connection with undo function. Repro:
A similar problem like this:
The second is more of an "uncommon workflow". Thanks for looking into it. |
|
@sqlkoala Thanks so much for testing this out! I'm not able to reproduce either one; would you mind submitting a new issue so we can track it more easily? |
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.
Many commands, like movement and typing a search, will never change the document. We waste considerable time diffing the entire document on each of these keystrokes to figure out if we should add an undo step.
I've added a property to BaseAction,
mightChangeDocument, which determines whether we should check for changes.This defaults to false for the sake of brevity (most commands actually don't change the document), but this is a bit dangerous - what if I forgot to override it in a command that changes the document?
The planned solution is to release this as a check - we will always run the diff, as we currently do, then make sure a command didn't add a change when we asserted it wouldn't. After this is in the wild for a little while with no reported issues, we'll actually flip the switch to optimize.
In my testing on a ~80,000 line file, this saves maybe 10-15ms per keystroke on average, which does translate into a noticeable improvement in responsiveness.
What this PR does / why we need it:
Which issue(s) this PR fixes
Special notes for your reviewer: