apple / swift-algorithms

Commonly used sequence and collection algorithms for Swift
Apache License 2.0
5.92k stars 440 forks source link

`min(count:…)` implementation: Remove the last element before determining the insertion index #173

Closed mdznr closed 2 years ago

mdznr commented 2 years ago

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:

  1. 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 (like Ints), 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.

  2. The insertion index that we found could theoretically be invalidated after calling removeLast(), according to the documentation:

    Calling this method may invalidate all saved indices of this collection. Do not rely on a previously stored index value after altering a collection with any operation that can change its length.

    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 than Array.

Neither of these are real big issues, but I stumbled upon this today and thought it a small worthwhile change.

Checklist

natecook1000 commented 2 years ago

@swift-ci Please test

natecook1000 commented 2 years ago

Thanks, @mdznr! Looks like a sound improvement.