[stdlib] Make UBP nil/count checks debug-mode only #34747
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Every other Unsafe(Mutable)BufferPointer precondition in this file is debug-mode only. This information is already part of the documentation for this function, and it's really hard to get rid of these branches and traps otherwise.
Contributor
Author
|
CC @airspeedswift as stdlib code owner. I'm not sure how to properly "request a review". |
Member
atrick
approved these changes
Nov 16, 2020
Contributor
atrick
left a comment
atrick
left a comment
There was a problem hiding this comment.
This change seems consistent with the intention of the UBP types. I can't come up with an argument against it.
Member
|
@swift-ci please test |
Member
|
@swift-ci please benchmark |
Contributor
|
Build failed before running benchmark. |
Contributor
|
Build failed |
Contributor
|
Build failed |
lorentey
approved these changes
Nov 18, 2020
Member
lorentey
left a comment
lorentey
left a comment
There was a problem hiding this comment.
This looks good! There should not be any performance drawback to using buffer pointers instead of naked pointers.
A tiny part of me wants to argue against this on some vague principle, but I'm failing to come up with any reasonable argument that wouldn't also obviously apply to subscripts. 😉
Member
|
Member
|
Member
|
The array test failure is triggered by this change -- it is actually testing one of these preconditions. It needs to have its skip condition updated to |
Member
|
It's somewhat disconcerting that there aren't more test failures -- it looks like we may not be adequately testing these checks. |
Member
|
The failing SILOptimizer test is this one: This is supposed to check that I wonder if the removal of the precondition led to the compiler randomly deciding to inline the @milseman Would it be okay if we disabled this particular check for _fromUTF8Repairing? FWIW, it looks like a subsequent check (for |
Contributor
Author
|
I misread the comment at the top of the unsafe-buffer validation tests and thought these weren't able to be tested. Of course, since this isn't an optimisation-only behaviour right now, it can be and apparently is tested 🤦♂️ I don't know what to do about the SILOptimizer test, so I'll wait for @milseman 's advice on that. It's nice to get rid of this check because basically every way of creating an UBP goes through it. The one possible exception that I've just thought of is the |
Member
|
|
Member
|
@swift-ci please benchmark |
Contributor
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
Member
|
@milseman @atrick I finally managed to reproduce this locally -- here is the emitted sil: // decodeUMRBPAsUTF8(_:)
sil @$s22utf8_decoding_fastpath17decodeUMRBPAsUTF8ySSSwF : $@convention(thin) (UnsafeMutableRawBufferPointer) -> @owned String {
// %0 "ptr" // users: %4, %1
bb0(%0 : $UnsafeMutableRawBufferPointer):
debug_value %0 : $UnsafeMutableRawBufferPointer, let, name "ptr", argno 1 // id: %1
%2 = alloc_stack $_HasContiguousBytes // users: %13, %12, %5, %3
%3 = init_existential_addr %2 : $*_HasContiguousBytes, $UnsafeMutableRawBufferPointer // user: %4
store %0 to %3 : $*UnsafeMutableRawBufferPointer // id: %4
%5 = open_existential_addr immutable_access %2 : $*_HasContiguousBytes to $*@opened("08EFEDA4-360C-11EB-9D6A-D0817AD8D2AD") _HasContiguousBytes // user: %7
// function_ref closure #2 in String.init<A, B>(decoding:as:)
%6 = function_ref @$sSS8decoding2asSSx_q_mtcSlRzs16_UnicodeEncodingR_8CodeUnitQy_7ElementRtzr0_lufcSSSWXEfU0_ : $@convention(thin) (UnsafeRawBufferPointer) -> @owned String // user: %11
%7 = unchecked_addr_cast %5 : $*@opened("08EFEDA4-360C-11EB-9D6A-D0817AD8D2AD") _HasContiguousBytes to $*UnsafeMutableRawBufferPointer // user: %8
%8 = load %7 : $*UnsafeMutableRawBufferPointer // user: %10
// function_ref specialized UnsafeRawBufferPointer.init(_:)
%9 = function_ref @$sSWySWSwcfCTf4nd_n : $@convention(thin) (UnsafeMutableRawBufferPointer) -> UnsafeRawBufferPointer // user: %10
%10 = apply %9(%8) : $@convention(thin) (UnsafeMutableRawBufferPointer) -> UnsafeRawBufferPointer // user: %11
%11 = apply %6(%10) : $@convention(thin) (UnsafeRawBufferPointer) -> @owned String // user: %14
destroy_addr %2 : $*_HasContiguousBytes // id: %12
dealloc_stack %2 : $*_HasContiguousBytes // id: %13
return %11 : $String // id: %14
} // end sil function '$s22utf8_decoding_fastpath17decodeUMRBPAsUTF8ySSSwF'The failing test is looking for As a reminder, the test is definitely just calling the decoding initializer with the UTF8 encoding: public func decodeUMRBPAsUTF8(_ ptr: UnsafeMutableRawBufferPointer) -> String {
return String(decoding: ptr, as: Unicode.UTF8.self)
}The problem is that I'm sure there is a good reason why changing a couple of What if we added the _onFastPath to the second closure, too? (Or removed it from both?) |
Member
|
With // decodeUMRBPAsUTF8(_:)
sil @$s22utf8_decoding_fastpath17decodeUMRBPAsUTF8ySSSwF : $@convention(thin) (UnsafeMutableRawBufferPointer) -> @owned String {
// %0 "ptr" // users: %4, %1
bb0(%0 : $UnsafeMutableRawBufferPointer):
debug_value %0 : $UnsafeMutableRawBufferPointer, let, name "ptr", argno 1 // id: %1
%2 = alloc_stack $_HasContiguousBytes // users: %29, %28, %5, %3
%3 = init_existential_addr %2 : $*_HasContiguousBytes, $UnsafeMutableRawBufferPointer // user: %4
store %0 to %3 : $*UnsafeMutableRawBufferPointer // id: %4
%5 = open_existential_addr immutable_access %2 : $*_HasContiguousBytes to $*@opened("09F1FD1C-3613-11EB-8F91-D0817AD8D2AD") _HasContiguousBytes // user: %6
%6 = unchecked_addr_cast %5 : $*@opened("09F1FD1C-3613-11EB-8F91-D0817AD8D2AD") _HasContiguousBytes to $*UnsafeMutableRawBufferPointer // user: %7
%7 = load %6 : $*UnsafeMutableRawBufferPointer // user: %9
// function_ref specialized UnsafeRawBufferPointer.init(_:)
%8 = function_ref @$sSWySWSwcfCTf4nd_n : $@convention(thin) (UnsafeMutableRawBufferPointer) -> UnsafeRawBufferPointer // user: %9
%9 = apply %8(%7) : $@convention(thin) (UnsafeMutableRawBufferPointer) -> UnsafeRawBufferPointer // users: %23, %11
%10 = builtin "onFastPath"() : $()
%11 = struct_extract %9 : $UnsafeRawBufferPointer, #UnsafeRawBufferPointer._position // user: %12
switch_enum %11 : $Optional<UnsafeRawPointer>, case #Optional.some!enumelt: bb1, case #Optional.none!enumelt: bb2 // id: %12
// %13 // user: %14
bb1(%13 : $UnsafeRawPointer): // Preds: bb0
%14 = struct_extract %13 : $UnsafeRawPointer, #UnsafeRawPointer._rawValue // user: %15
%15 = struct $UnsafePointer<UInt8> (%14 : $Builtin.RawPointer) // user: %16
%16 = enum $Optional<UnsafePointer<UInt8>>, #Optional.some!enumelt, %15 : $UnsafePointer<UInt8> // user: %17
br bb3(%16 : $Optional<UnsafePointer<UInt8>>) // id: %17
bb2: // Preds: bb0
%18 = enum $Optional<UnsafePointer<UInt8>>, #Optional.none!enumelt // user: %19
br bb3(%18 : $Optional<UnsafePointer<UInt8>>) // id: %19
// %20 // user: %24
bb3(%20 : $Optional<UnsafePointer<UInt8>>): // Preds: bb2 bb1
%21 = metatype $@thin String.Type // user: %26
// function_ref UnsafeRawBufferPointer.count.getter
%22 = function_ref @$sSW5countSivg : $@convention(method) (UnsafeRawBufferPointer) -> Int // user: %23
%23 = apply %22(%9) : $@convention(method) (UnsafeRawBufferPointer) -> Int // user: %24
%24 = struct $UnsafeBufferPointer<UInt8> (%20 : $Optional<UnsafePointer<UInt8>>, %23 : $Int) // user: %26
// function_ref static String._fromUTF8Repairing(_:)
%25 = function_ref @$sSS18_fromUTF8RepairingySS6result_Sb11repairsMadetSRys5UInt8VGFZ : $@convention(method) (UnsafeBufferPointer<UInt8>, @thin String.Type) -> (@owned String, Bool) // user: %26
%26 = apply %25(%24, %21) : $@convention(method) (UnsafeBufferPointer<UInt8>, @thin String.Type) -> (@owned String, Bool) // user: %27
%27 = tuple_extract %26 : $(String, Bool), 0 // user: %30
destroy_addr %2 : $*_HasContiguousBytes // id: %28
dealloc_stack %2 : $*_HasContiguousBytes // id: %29
return %27 : $String // id: %30
} // end sil function '$s22utf8_decoding_fastpath17decodeUMRBPAsUTF8ySSSwF'I submitted PR #34961 that includes this change along with #34951 and what we learned from #34879. |
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Currently: Creating a buffer with a negative
count, or anilpointer and non-zerocount(in violation of the initialiser's documentation) traps in every build mode. This is unfortunate, since these conditions are difficult for the compiler to prove and eliminate.After this change: Violating the documentation is a user error and will only trap in debug mode. As with other operations on unsafe buffers (e.g. out of bounds subscripting), they will not trap in release mode.