-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Insertmode performance improvement #9872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Insertmode performance improvement #9872
Conversation
…ad of 11) at iteration that happens at base.ts line 267 at time of writting 'for (const actionType of possibleActionsForMode)'. BackspaceInInsertMode was also moved as that has be handled before TypeInInsertMode.
| } | ||
| } | ||
|
|
||
| @RegisterAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for moving the methods? It makes it more difficult to tell what changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the only change that was made here was the move itself - only change of location and not of contents - This change causes the "TypeInInsertMode" action to get identified earlier (at iteration 6 instead of 11) at iteration that happens at base.ts line 267
src/configuration/langmap.ts
Outdated
| Mode.CommandlineInProgress, | ||
| Mode.SearchInProgressMode, | ||
| ]; | ||
| const literalModesMap = new Map<Mode, boolean>([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a Set rather than a Map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that is a better approach - I changed it
| : -1, | ||
| ); | ||
| const cursorsToIterateOver = | ||
| vimState.cursors.length > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you measured a performance difference with these changes? I want to be sure we're not prematurely optimizing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried - but could not create reproduceable measurements. I understand
What this PR does / why we need it:
This is a pr to improve "typing while in insert mode" performance.
This is effectively a refactor since that the changes do not introduce any behavioural changes - only increase performance with existing behaviour.
The changes (per the commit messages) do the following:
Which issue(s) this PR fixes
No issues has been created that I know of - I just profiled the "typing while in insertmode" code-path and added low-impact optimizations.
Special notes for your reviewer:
Submitting this to contribute to this great extension and fix issues I experience myself.