feat(im-util): SortBy optimises Remove + Insert to Set in another case
#49
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
…ther case. In the `SortBy` stream adapter, when the user is applying a `observable_vector.set(…)` that should result in a `Remove` + `Insert`, it sometimes can be optimised into a single `Set`. This happens when the `old_index` and the `new_index` (resp. the sorted index of the value to be updated, and the sorted index of the new value) are equal. But actually, when `old_index` is lower than `new_index`, there is _another_ missed optimisation in one particular context. If `old_index == new_index - 1`, then we are actually updating the same location. Indeed, the code is already making it clear: when `old_index < new_index`, we need to subtract 1 to `new_index` because removing the value at `old_index` is shifting all values at its right, thus 1 must be subtracted to `new_index`. The missed opportunity for an optimisation here is when `old_index == new_index - 1`, where it's clear that the same position is updated. This patch handles this situation. The tests are updated accordingly.
jplatte
approved these changes
Mar 14, 2024
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.
In the
SortBystream adapter, when the user is applying aobservable_vector.set(…)that should result in aRemove+Insert, it sometimes can be optimised into a singleSet. This happens when theold_indexand thenew_index(resp. the sorted index of the value to be updated, and the sorted index of the new value) are equal. But actually, whenold_indexis lower thannew_index, there is another missed optimisation in one particular context. Ifold_index == new_index - 1, then we are actually updating the same location. Indeed, the code is already making it clear: whenold_index < new_index, we need to subtract 1 tonew_indexbecause removing the value atold_indexis shifting all values at its right, thus 1 must be subtracted tonew_index. The missed opportunity for an optimisation here is whenold_index == new_index - 1, where it's clear that the same position is updated. This patch handles this situation. The tests are updated accordingly.Thanks @jplatte for having found this in #46 (comment).