min(count:…)
implementation: Remove the last element before determining the insertion index
#173
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.
Previously, in
_minImplementation(count:sortedBy:)
, when finding a new, smaller element, we were calculating the insertion index on the full-sized collection, subsequently removing the last element, then inserting the new element at that insertion index. This has a couple small problems:We were calling
partitioningIndex(where:)
on a larger collection than necessary. By removing the last element first (which doesn’t need to be compared against again), we make the collection slightly smaller, resulting in fewer comparisons. For simple comparisons (likeInt
s), this isn’t a big deal, but if the comparison is more expensive (like with a custom struct with multiple properties), this can make a slight difference.The insertion index that we found could theoretically be invalidated after calling
removeLast()
, according to the documentation:While this isn’t currently/ever going to be a problem for
Array
, it‘s still good practice. This could theoretically set us up in the future to use a more optimized data structure with different indexing behavior thanArray
.Neither of these are real big issues, but I stumbled upon this today and thought it a small worthwhile change.
Checklist