Skip to content

[stdlib] Collection: simplify default _failEarlyRangeCheck implementations #41548

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
merged 1 commit into from
Feb 28, 2022

Conversation

lorentey
Copy link
Member

@lorentey lorentey commented Feb 24, 2022

Currently, the default implementations of the various _failEarlyRangeCheck forms contain several _precondition invocations, like this:

@inlinable
  public func _failEarlyRangeCheck(_ index: Index, bounds: Range<Index>) {
    _precondition(
      bounds.lowerBound <= index,
      "Out of bounds: index < startIndex")
    _precondition(
      index < bounds.upperBound,
      "Out of bounds: index >= endIndex")
  }

Each such precondition call generates a separate trap instruction, which seems like a waste — theoretically it would be helpful to know which condition was violated, but in practice, that information tends not to be surfaced anyway. Combining these will lead to a minuscule code size improvement.

(The separate messages do surface these in debug builds, but only if these generic defaults actually execute, which isn’t often. These are not public entry points, so concrete collection types tend ignore these and roll their own custom index validation instead. From what I’ve seen, custom code tends to combine the upper/lower checks into a single precondition check. These underscored requirements tend to be only called by the standard Slice; and it seems reasonable for that to follow suit.)

More interestingly, the range-in-range version of _failEarlyRangeCheck performs two times as many checks than are necessary:

  @inlinable
  public func _failEarlyRangeCheck(_ range: Range<Index>, bounds: Range<Index>) {
    _precondition(
      bounds.lowerBound <= range.lowerBound,
      "Out of bounds: range begins before startIndex")
    _precondition(
      range.lowerBound <= bounds.upperBound,
      "Out of bounds: range ends after endIndex")
    _precondition(
      bounds.lowerBound <= range.upperBound,
      "Out of bounds: range ends before bounds.lowerBound")
    _precondition(
      range.upperBound <= bounds.upperBound,
      "Out of bounds: range begins after bounds.upperBound")
  }

It is safe to assume that range.lowerBound <= range.upperBound, so it’s enough to test that bounds.lowerBound <= range.lowerBound && range.upperBound <= bounds.upperBound — we don’t need to check each combination separately.

@lorentey
Copy link
Member Author

@swift-ci test

@lorentey
Copy link
Member Author

@natecook1000 @timvermeulen Any objections to collapsing these separate preconditions? I can see arguments either way, but the extra information they provide seems to be of marginal use -- it is usually easy to recover by debugging. It does make crash logs capture slightly less information. (When it proves important, I think the data will usually still be recoverable from register dumps (when available).)

(Hopefully the PR test will fail in some crash test that looks at the error messages. If not, then we should do something about those fixmes...)

@lorentey
Copy link
Member Author

Collection.swift:732:34: error: value of type 'Range<Self.Index>' has no member 'upperbound'

Oops

…tions

Currently, the default implementations of the various `_failEarlyRangeCheck` forms contain several _precondition invocations, like this:

```
@inlinable
  public func _failEarlyRangeCheck(_ index: Index, bounds: Range<Index>) {
    _precondition(
      bounds.lowerBound <= index,
      "Out of bounds: index < startIndex")
    _precondition(
      index < bounds.upperBound,
      "Out of bounds: index >= endIndex")
  }
```

Each such precondition call generates a separate trap instruction, which seems like a waste — theoretically it would be helpful to know which condition was violated, but in practice, that information tends not to be surfaced anyway. Combining these will lead to a minuscule code size improvement.

(The separate messages do surface these in debug builds, but only if these generic defaults actually execute, which isn’t often. These are not public entry points, so concrete collection types tend ignore these and roll their own custom index validation instead. From what I’ve seen, custom code tends to combine the upper/lower checks into a single precondition check. These underscored requirements tend to be only called by the standard `Slice`; and it seems reasonable for that to follow suit.)

More interestingly, the range-in-range version of `_failEarlyRangeCheck` performs two times as many checks than are necessary:

```
  @inlinable
  public func _failEarlyRangeCheck(_ range: Range<Index>, bounds: Range<Index>) {
    _precondition(
      bounds.lowerBound <= range.lowerBound,
      "Out of bounds: range begins before startIndex")
    _precondition(
      range.lowerBound <= bounds.upperBound,
      "Out of bounds: range ends after endIndex")
    _precondition(
      bounds.lowerBound <= range.upperBound,
      "Out of bounds: range ends before bounds.lowerBound")
    _precondition(
      range.upperBound <= bounds.upperBound,
      "Out of bounds: range begins after bounds.upperBound")
  }
```

It is safe to assume that `range.lowerBound <= range.upperBound`, so it’s enough to test that `bounds.lowerBound <= range.lowerBound && range.upperBound <= bounds.upperBound` — we don’t need to check each combination separately.
@lorentey lorentey force-pushed the simplify-failEarlyRangeCheck branch from 9fdd04e to 5293bde Compare February 26, 2022 01:06
@lorentey
Copy link
Member Author

@swift-ci test

@lorentey
Copy link
Member Author

(Not expecting any major changes, but ¯\_(ツ)_/¯)

@swift-ci benchmark

@lorentey
Copy link
Member Author

@swift-ci benchmark

@lorentey
Copy link
Member Author

Cancelling nested steps due to timeout
Sending interrupt signal to process
ninja: build stopped: interrupted by user.
/Users/ec2-user/jenkins/workspace/swift-PR-macos/branch-main/swift/utils/build-script-impl: line 303: 43107 Terminated: 15          "$@"

@lorentey
Copy link
Member Author

@swift-ci test macOS platform

@lorentey lorentey merged commit 57af818 into swiftlang:main Feb 28, 2022
@lorentey lorentey deleted the simplify-failEarlyRangeCheck branch February 28, 2022 22:52
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.

1 participant