Remove checks in UR[M]BP.Iterator.next() #62965
Conversation
|
@swift-ci please smoke test |
Lukasa
force-pushed
the
cb-better-urbp-iteration
branch
from
1d708ae to
85c719d
Compare
| // | ||
| // We check these invariants in debug builds to defend against invalidly constructed | ||
| // pointers. | ||
| _debugPrecondition(_position! <= _end!) |
There was a problem hiding this comment.
< rather than <=; we already eliminated == above, and < is what guarantees that we can use &+=.
|
@Lukasa let's clone this for 5.8 once we're happy with it. |
Lukasa
force-pushed
the
cb-better-urbp-iteration
branch
from
85c719d to
6a37833
Compare
|
@swift-ci please smoke test |
|
@stephentyrone Yup, will do. |
|
@swift-ci please smoke test |
Lukasa
force-pushed
the
cb-better-urbp-iteration
branch
from
6a37833 to
6fdb7cd
Compare
|
@swift-ci please smoke test |
Lukasa
force-pushed
the
cb-better-urbp-iteration
branch
from
6fdb7cd to
ece32d4
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
@swift-ci please benchmark |
|
Hmm, any idea why the benchmark runner is failing? |
| let result = _position!.load(as: UInt8.self) | ||
| _position! += 1 | ||
| // We can do an unchecked unwrap here by borrowing invariants from the pointer. | ||
| // For a validly constructed buffer pointer, he only way _position can be nil is |
Swift tends to emit unnecessary checks and traps when iterating unsafe raw buffer pointers. These traps are confirming that the position pointer isn't nil, but this check is redundant with the bounds check that is already present. We can safely remove it.
Lukasa
force-pushed
the
cb-better-urbp-iteration
branch
from
ece32d4 to
d9d9e0f
Compare
|
@swift-ci please test |
|
@swift-ci please benchmark |
| // | ||
| // We check these invariants in debug builds to defend against invalidly constructed | ||
| // pointers. | ||
| _debugPrecondition(_position! < _end!) |
There was a problem hiding this comment.
Couldn't this be hoisted in to the iterator's initializer, as a check that _position <= _end? That would give us the same assurances, but IMO would be a bit cleaner; we wouldn't need to talk about 'validly constructed buffer pointer's any more, more like 'validly constructed iterators'. Additionally, it may improve performance in debug builds, which is also important.
There was a problem hiding this comment.
There's value in validating invariants outside of the initializer, to detect bugs that corrupt them sooner rather than later. Looking at the codegen, this doesn't cost us much, so it should probably stick around (but I would prefer they be _unsafelyUnwrappedUnchecked)
There was a problem hiding this comment.
@stephentyrone Would you like a != nil _debugPrecondition as well?
There was a problem hiding this comment.
I think I'm fine with it as you have it.
| @@ -148,8 +148,21 @@ extension UnsafeRawBufferPointer.Iterator: IteratorProtocol, Sequence { | |||
| public mutating func next() -> UInt8? { | |||
| if _position == _end { return nil } | |||
There was a problem hiding this comment.
This should compile to a single comparison, but doesn't. I'm opening an LLVM bug to see what we can do about that.
Swift tends to emit unnecessary checks and traps when iterating unsafe raw buffer pointers. These traps are confirming that the position pointer isn't nil, but this check is redundant with the bounds check that is already present. We can safely remove it.
Resolves #62964.