Skip to content

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

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

Merged

Conversation

mdznr
Copy link
Contributor

@mdznr mdznr commented Nov 18, 2021

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

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

This could result in fewer comparisons.

Additionally, it ensures that the insertion index is valid for insertion. The `removeLast()` documentation states:
> 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.
@mdznr mdznr marked this pull request as ready for review November 18, 2021 21:20
@natecook1000
Copy link
Member

@swift-ci Please test

@natecook1000
Copy link
Member

Thanks, @mdznr! Looks like a sound improvement.

@natecook1000 natecook1000 merged commit 19c1d00 into apple:main Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants