Skip to content

Commit

Permalink
Improve performance on large files by not checking for changes after …
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@J-Fields
J-Fields committed Jan 1, 2020
1 parent 71375dd commit abc2afd
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 4 deletions.
14 changes: 14 additions & 0 deletions src/actions/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,20 @@ export class BaseAction {
private static readonly isSingleNumber: RegExp = /^[0-9]$/;
private static readonly isSingleAlpha: RegExp = /^[a-zA-Z]$/;

/**
* True if this action has the potential to change the document's text, and therefore trigger an
* undo step. This is for optimization purposes - we skip the expensive process of figuring out
* what changed to create that undo step if this returns false.
*
* Most commands do not modify the document, so this is false by default. This means if you add a
* command that can modify the document, you MUST set mightChangeDocument = true. Otherwise undo
* will not work properly.
*
* TODO: if all actions were pure, I think this would be unnecessary, as we could deduce it from
* vimState.transformations being empty or not.
*/
public mightChangeDocument: boolean = false;

/**
* Is this action valid in the current Vim state?
*/
Expand Down
48 changes: 47 additions & 1 deletion src/actions/commands/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export class DocumentContentChangeAction extends BaseAction {
positionDiff: PositionDiff;
textDiff: vscode.TextDocumentContentChangeEvent;
}[] = [];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
if (this.contentChanges.length === 0) {
Expand Down Expand Up @@ -510,6 +511,7 @@ class CommandExecuteMacro extends BaseCommand {
keys = ['@', '<character>'];
runsOnceForEachCountPrefix = true;
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const register = this.keysPressed[1];
Expand Down Expand Up @@ -546,6 +548,7 @@ class CommandExecuteLastMacro extends BaseCommand {
runsOnceForEachCountPrefix = true;
canBeRepeatedWithDot = true;
isJump = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
let lastInvokedMacro = vimState.historyTracker.lastInvokedMacro;
Expand Down Expand Up @@ -624,6 +627,7 @@ class CommandEsc extends BaseCommand {
class CommandEscReplaceMode extends BaseCommand {
modes = [Mode.Replace];
keys = [['<Esc>'], ['<C-c>'], ['<C-[>']];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const timesToRepeat = vimState.replaceState!.timesToRepeat;
Expand Down Expand Up @@ -859,6 +863,7 @@ class CommandReplaceInReplaceMode extends BaseCommand {
modes = [Mode.Replace];
keys = ['<character>'];
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const char = this.keysPressed[0];
Expand Down Expand Up @@ -1433,6 +1438,7 @@ export class PutCommand extends BaseCommand {
modes = [Mode.Normal];
runsOnceForEachCountPrefix = true;
canBeRepeatedWithDot = true;
mightChangeDocument = true;

constructor(multicursorIndex?: number) {
super();
Expand Down Expand Up @@ -1718,6 +1724,7 @@ export class GPutCommand extends BaseCommand {
modes = [Mode.Normal, Mode.Visual, Mode.VisualLine];
runsOnceForEachCountPrefix = true;
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
return new PutCommand().exec(position, vimState);
Expand Down Expand Up @@ -1763,6 +1770,7 @@ export class PutWithIndentCommand extends BaseCommand {
modes = [Mode.Normal, Mode.Visual, Mode.VisualLine];
runsOnceForEachCountPrefix = true;
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const result = await new PutCommand().exec(position, vimState, false, true);
Expand All @@ -1779,6 +1787,7 @@ export class PutCommandVisual extends BaseCommand {
keys = [['p'], ['P']];
modes = [Mode.Visual, Mode.VisualLine];
runsOnceForEachCountPrefix = true;
mightChangeDocument = true;

public async exec(
position: Position,
Expand Down Expand Up @@ -1852,6 +1861,7 @@ export class PutBeforeCommand extends BaseCommand {
public modes = [Mode.Normal];
canBeRepeatedWithDot = true;
runsOnceForEachCountPrefix = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const command = new PutCommand();
Expand All @@ -1867,6 +1877,7 @@ export class PutBeforeCommand extends BaseCommand {
export class GPutBeforeCommand extends BaseCommand {
keys = ['g', 'P'];
modes = [Mode.Normal];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const result = await new PutCommand().exec(position, vimState, true);
Expand Down Expand Up @@ -1904,6 +1915,7 @@ export class GPutBeforeCommand extends BaseCommand {
export class PutBeforeWithIndentCommand extends BaseCommand {
keys = ['[', 'p'];
modes = [Mode.Normal];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const result = await new PutCommand().exec(position, vimState, true, true);
Expand Down Expand Up @@ -2093,7 +2105,7 @@ class CommandTabInCommandline extends BaseCommand {
}
}

// TODO: Split this into multiple commands and maybe move them to another file
// TODO: Split this into multiple commands and maybe move them to another file (mightChangeDocument can be false for some!)
@RegisterAction
class CommandInsertInCommandline extends BaseCommand {
modes = [Mode.CommandlineInProgress];
Expand All @@ -2115,6 +2127,7 @@ class CommandInsertInCommandline extends BaseCommand {
runsOnceForEveryCursor() {
return this.keysPressed[0] === '\n';
}
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const key = this.keysPressed[0];
Expand Down Expand Up @@ -2326,6 +2339,7 @@ export class CommandShowSearchHistory extends BaseCommand {
class CommandDot extends BaseCommand {
modes = [Mode.Normal];
keys = ['.'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
vimState.recordedState.transformations.push({
Expand All @@ -2340,6 +2354,7 @@ class CommandDot extends BaseCommand {
class CommandRepeatSubstitution extends BaseCommand {
modes = [Mode.Normal];
keys = ['&'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
// Parsing the command from a string, while not ideal, is currently
Expand Down Expand Up @@ -2631,6 +2646,7 @@ class CommandUndo extends BaseCommand {
return false;
}
mustBeFirstKey = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const newPositions = await vimState.historyTracker.goBackHistoryStep();
Expand All @@ -2655,6 +2671,7 @@ class CommandUndoOnLine extends BaseCommand {
return false;
}
mustBeFirstKey = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const newPositions = await vimState.historyTracker.goBackHistoryStepsOnLine();
Expand All @@ -2672,6 +2689,7 @@ class CommandUndoOnLine extends BaseCommand {
class CommandRedo extends BaseCommand {
modes = [Mode.Normal];
keys = ['<C-r>'];
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand All @@ -2695,6 +2713,7 @@ class CommandDeleteToLineEnd extends BaseCommand {
modes = [Mode.Normal];
keys = ['D'];
canBeRepeatedWithDot = true;
mightChangeDocument = true;
runsOnceForEveryCursor() {
return true;
}
Expand Down Expand Up @@ -2733,6 +2752,7 @@ class CommandChangeToLineEnd extends BaseCommand {
modes = [Mode.Normal];
keys = ['C'];
runsOnceForEachCountPrefix = false;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const count = vimState.recordedState.count || 1;
Expand All @@ -2753,6 +2773,7 @@ class CommandClearLine extends BaseCommand {
modes = [Mode.Normal];
keys = ['S'];
runsOnceForEachCountPrefix = false;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
return new operator.ChangeOperator(this.multicursorIndex).runRepeat(
Expand Down Expand Up @@ -3187,6 +3208,7 @@ export class CommandInsertAtLineEnd extends BaseCommand {
class CommandInsertNewLineAbove extends BaseCommand {
modes = [Mode.Normal];
keys = ['O'];
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand Down Expand Up @@ -3226,6 +3248,7 @@ class CommandInsertNewLineAbove extends BaseCommand {
class CommandInsertNewLineBefore extends BaseCommand {
modes = [Mode.Normal];
keys = ['o'];
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand Down Expand Up @@ -3544,6 +3567,7 @@ export class ActionDeleteChar extends BaseCommand {
modes = [Mode.Normal];
keys = ['x'];
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
// If line is empty, do nothing
Expand Down Expand Up @@ -3571,6 +3595,7 @@ export class ActionDeleteCharWithDeleteKey extends BaseCommand {
keys = ['<Del>'];
runsOnceForEachCountPrefix = true;
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async execCount(position: Position, vimState: VimState): Promise<VimState> {
// If <del> has a count in front of it, then <del> deletes a character
Expand All @@ -3592,6 +3617,7 @@ export class ActionDeleteLastChar extends BaseCommand {
modes = [Mode.Normal];
keys = ['X'];
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
if (position.character === 0) {
Expand All @@ -3616,6 +3642,7 @@ class ActionJoin extends BaseCommand {
keys = ['J'];
canBeRepeatedWithDot = true;
runsOnceForEachCountPrefix = false;
mightChangeDocument = true;

private firstNonWhitespaceIndex(str: string): number {
for (let i = 0, len = str.length; i < len; i++) {
Expand Down Expand Up @@ -3779,6 +3806,7 @@ class ActionJoin extends BaseCommand {
class ActionJoinVisualMode extends BaseCommand {
modes = [Mode.Visual, Mode.VisualLine];
keys = ['J'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
let actionJoin = new ActionJoin();
Expand All @@ -3805,6 +3833,7 @@ class ActionJoinNoWhitespace extends BaseCommand {
keys = ['g', 'J'];
canBeRepeatedWithDot = true;
runsOnceForEachCountPrefix = true;
mightChangeDocument = true;

// gJ is essentially J without the edge cases. ;-)

Expand Down Expand Up @@ -3848,6 +3877,7 @@ class ActionJoinNoWhitespace extends BaseCommand {
class ActionJoinNoWhitespaceVisualMode extends BaseCommand {
modes = [Mode.Visual];
keys = ['g', 'J'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
let actionJoin = new ActionJoinNoWhitespace();
Expand Down Expand Up @@ -3876,6 +3906,7 @@ class ActionReplaceCharacter extends BaseCommand {
keys = ['r', '<character>'];
canBeRepeatedWithDot = true;
runsOnceForEachCountPrefix = false;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
let timesToRepeat = vimState.recordedState.count || 1;
Expand Down Expand Up @@ -3951,6 +3982,7 @@ class ActionReplaceCharacterVisual extends BaseCommand {
return false;
}
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
let toInsert = this.keysPressed[1];
Expand Down Expand Up @@ -4034,6 +4066,7 @@ class ActionReplaceCharacterVisualBlock extends BaseCommand {
return false;
}
canBeRepeatedWithDot = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
let toInsert = this.keysPressed[1];
Expand Down Expand Up @@ -4075,6 +4108,7 @@ class ActionXVisualBlock extends BaseCommand {
runsOnceForEveryCursor() {
return false;
}
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const lines: string[] = [];
Expand Down Expand Up @@ -4106,9 +4140,11 @@ class ActionXVisualBlock extends BaseCommand {

@RegisterAction
class ActionDVisualBlock extends ActionXVisualBlock {
// TODO: can't this be lumped in with the previous command without subclassing?
modes = [Mode.VisualBlock];
keys = ['d'];
canBeRepeatedWithDot = true;
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand All @@ -4122,6 +4158,7 @@ class ActionShiftDVisualBlock extends BaseCommand {
runsOnceForEveryCursor() {
return false;
}
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
for (const { start } of Position.IterateLine(vimState)) {
Expand Down Expand Up @@ -4172,6 +4209,7 @@ class ActionGoToInsertVisualBlockMode extends BaseCommand {
class ActionChangeInVisualBlockMode extends BaseCommand {
modes = [Mode.VisualBlock];
keys = [['c'], ['s']];
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand Down Expand Up @@ -4202,6 +4240,7 @@ class ActionChangeInVisualBlockMode extends BaseCommand {
class ActionChangeToEOLInVisualBlockMode extends BaseCommand {
modes = [Mode.VisualBlock];
keys = [['C'], ['S']];
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand Down Expand Up @@ -4328,6 +4367,7 @@ export class ActionGoToInsertVisualModeAppend extends ActionGoToInsertVisualLine
class ActionGoToInsertVisualBlockModeAppend extends BaseCommand {
modes = [Mode.VisualBlock];
keys = ['A'];
mightChangeDocument = true;
runsOnceForEveryCursor() {
return false;
}
Expand Down Expand Up @@ -4357,6 +4397,7 @@ class ActionGoToInsertVisualBlockModeAppend extends BaseCommand {
class ActionDeleteLineVisualMode extends BaseCommand {
modes = [Mode.Visual, Mode.VisualLine];
keys = ['X'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
if (vimState.currentMode === Mode.Visual) {
Expand All @@ -4379,6 +4420,7 @@ class ActionDeleteLineVisualMode extends BaseCommand {
class ActionChangeLineVisualMode extends BaseCommand {
modes = [Mode.Visual];
keys = ['C'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
return new operator.DeleteOperator(this.multicursorIndex).run(
Expand All @@ -4393,6 +4435,7 @@ class ActionChangeLineVisualMode extends BaseCommand {
class ActionRemoveLineVisualMode extends BaseCommand {
modes = [Mode.Visual];
keys = ['R'];
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
return new operator.DeleteOperator(this.multicursorIndex).run(
Expand All @@ -4408,6 +4451,7 @@ class ActionChangeChar extends BaseCommand {
modes = [Mode.Normal];
keys = ['s'];
runsOnceForEachCountPrefix = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const state = await new operator.ChangeOperator().run(vimState, position, position);
Expand Down Expand Up @@ -4441,6 +4485,7 @@ class ToggleCaseAndMoveForward extends BaseCommand {
keys = ['~'];
canBeRepeatedWithDot = true;
runsOnceForEachCountPrefix = true;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
await new operator.ToggleCaseOperator().run(
Expand All @@ -4458,6 +4503,7 @@ abstract class IncrementDecrementNumberAction extends BaseCommand {
modes = [Mode.Normal];
canBeRepeatedWithDot = true;
offset: number;
mightChangeDocument = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
const text = TextEditor.getLineAt(position).text;
Expand Down
Loading

0 comments on commit abc2afd

Please sign in to comment.