Look up the displaced action by command id in KeyBindingService - #4243
Open
vogella wants to merge 1 commit into
Open
Look up the displaced action by command id in KeyBindingService#4243vogella wants to merge 1 commit into
vogella wants to merge 1 commit into
Conversation
registerAction scanned every already registered action to find the one holding the command id it is about to take over. AbstractTextEditor registers dozens of actions per editor through setAction, so registering n actions cost O(n^2) comparisons on every editor open. Keep a map from command id to action, and its inverse for removal, so the lookup is a single map access. The scan also removed entries from the keySet it was iterating and only avoided a ConcurrentModificationException because of the break directly after, which the map lookup makes moot. The maps are keyed by the command id an action was registered under rather than by its current definition id. These differ only if getActionDefinitionId changes after registration, in which case eviction now matches the id the handler activation actually uses instead of one the activation was never created for. Add KeyBindingServiceTest, which pins down that registering an action for an already bound command id displaces the previous action, and that the displaced action is deactivated rather than shadowed, so it does not resurface once the replacement is unregistered. That behaviour had no coverage. The tests pass unchanged against the previous implementation. This is not a measurable speedup. Timed with OpenCloseEditorTest over three alternating runs per variant, the difference stays under the run to run spread of the baseline itself.
Contributor
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
KeyBindingService.registerActionscanned every already registered action to find the one currently holding the command id it is about to take over.AbstractTextEditorregisters dozens of actions per editor viasetAction, so registering n actions costs O(n^2) comparisons on every editor open. A map from command id to action, plus its inverse for removal, turns that into a single lookup. As a side effect the scan no longer removes entries from thekeySetit is iterating, which previously only avoided aConcurrentModificationExceptionbecause of thebreakimmediately after it.The maps are keyed by the command id an action was registered under rather than by its current definition id. Those differ only when
getActionDefinitionIdchanges after registration, and in that case eviction now matches the id the handler activation actually uses, rather than one the activation was never created for.The PR also adds
KeyBindingServiceTest. Registering an action for an already bound command id displaces the previous action, and the displaced one has to be deactivated rather than shadowed so it does not resurface when the replacement is unregistered. None of that had any coverage before. The tests pass unchanged against the previous implementation, so they document existing behaviour rather than the refactor.To be upfront about the performance side: this is not a measurable win. Timed with
OpenCloseEditorTestover three alternating runs per variant, the difference stays below the run to run spread of the baseline itself. The value here is the removed quadratic loop, the removed iteration hazard, and the new test coverage.