[stdlib] Random unification #16413
[stdlib] Random unification #16413
Conversation
Initial random api
Use C syscall for I/O
1. Fixed an issue where integers would would result in an infinite loop if they were unsigned, or signed integers always returning negative numbers.
2. Fixed an issue with Bool initialization
Add shuffle functions
Add documentation to Random API
Fix a few typos within the documentation
Fixes more typos
Also states that the range for floating points is from 0 to 1 inclusive
Update API to reflect mailing list discussions
Remove unnecessary import
Make sure not to return upperBound on Range
Use SecRandomCopyBytes on older macOS
Update API to match mailing list discussion, add tests
Added pick(_:) to collection
Added random(in:using:) to Randomizable
Added tests
Fix typo in Randomizable documentation
Rename pick to sampling
Move sampling below random
Update docs
Use new Libc naming
Fix Random.swift with new Libc naming
Remove sampling
gybify signed integer creation
Make FloatingPoint.random exclusive
Refactor {Closed}Range.random
Fix FloatingPoint initialization
Precondition getting a random number from range
Fix some doc typos
Make .random a function
Update API to reflect discussion
Make .random a function
Remove .random() in favor of .random(in:) for all numeric types
Fix compile errors
Clean up _stdlib_random
Cleanup around API
Remove `.random()` requirement from `Collection`
Use generators
Optimize shuffle()
Thread safety for /dev/urandom
Remove {Closed}Range<BinaryFloatingPoint>.random()
Add Collection random requirement
Refactor _stdlib_random
Remove whitespace changes
Clean linux shim
Add shuffle and more tests
Provide finishing tests and suggestions
Remove refs to Countable ranges
Revert to checking if T is > UInt64
* Remove refs to Countable ranges * Add `_stdlib_random` for more platforms * Use `getrandom` (if available) for Android, Cygwin * Reorder the `_stdlib_random` functions * Also include <features.h> on Linux * Add `#error TODO` in `_stdlib_random` for Windows * Colon after Fatal Error Performance improvement for Random gybify ranges Fix typo in 'basic random numbers' Add _stdlib_random as a testable method Switch to generic constraints Hopefully link against bcrypt Fix some implementation details 1. Uniform distribution is now uniform 2. Apply Jens' method for uniform floats Fix a lineable attribute
* [stdlib] Revise documentation for new random APIs * [stdlib] Fix constraints on random integer generation * [test] Isolate failing Random test * [benchmark] Add benchmarks for new random APIs Fix Float80 test Value type generators random -> randomElement Fix some docs One more doc fix Doc fixes & bool fix Use computed over explicit
|
cc: @airspeedswift |
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke benchmark |
natecook1000
left a comment
natecook1000
left a comment
There was a problem hiding this comment.
I just have this one Q, then LGTM pending tests. I have some more documentation edits ready to go, but let's get this merged first!
stdlib/public/stubs/LibcShims.cpp
Outdated
There was a problem hiding this comment.
Is this going to break the build for Win32 platforms?
There was a problem hiding this comment.
Yes and no, this does break builds if they're compiling the stdlib. I can probably substitute this in with a warning directive instead.
Build comment file:Optimized (O)Regression (17)
Improvement (18)
No Changes (392)
Added (6)
Unoptimized (Onone)Regression (23)
Improvement (27)
No Changes (377)
Added (6)
Hardware Overview |
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
I wonder if it would make sense to split off the rest of this function into a non-@inlinable @inline(never) internal method, and _fastPath the branch above. This could make it more likely that the 99.99% use case and fast-path is more reliably inlined and/or specialized. CC @eeckstein for thoughts.
Also, how is the rest of this code tested?
There was a problem hiding this comment.
As for testing the rest, we currently do not test it, which is a concern @benrimmington brought up. Are there any testing mechanisms for larger integers (such as UInt72), or do we need to write up one?
There was a problem hiding this comment.
We have the DoubleWidth type here, we could test a 128-bit integer type with that: https://github.com/apple/swift/blob/master/test/Prototypes/DoubleWidth.swift.gyb
If we want to test anything that isn't a power of two wide, that'll be an additional thing to develop.
There was a problem hiding this comment.
Does FixedWidthInteger actually require that the representable range be 0 ..< 2^N for some N? It probably should, but I can't remember that it's actually spelled out anywhere, and this implementation assumes it.
There was a problem hiding this comment.
Luckily, the static FixedWidthInteger.bitWidth property has the following note:
An unsigned, fixed-width integer type can represent values from
0through(2 ** bitWidth) - 1, where**is exponentiation.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Could you add a comment explaining the implementation strategy and the reason you chose it?
There was a problem hiding this comment.
FWIW, in arc4random_uniform, we choose the smallest N such that 2^N is greater than or equal to upperBound, and then pull N bits at a time from the random source until we get a number smaller than upperBound. This avoids needing to compute two % operations, which is pretty significant.
stdlib/public/stubs/LibcShims.cpp
Outdated
stdlib/public/stubs/LibcShims.cpp
Outdated
There was a problem hiding this comment.
I'm not sure if this code should apply to arc4random on Darwin platforms, which claims it never returns an error code.
There was a problem hiding this comment.
This does not apply to arc4random on Darwin platforms and won't be executed during runtime. I can separate Darwin's solution outside of this one if that clears any confusion.
stdlib/public/stubs/LibcShims.cpp
Outdated
There was a problem hiding this comment.
Nit: Please re-outdent the conditional preprocessor directives.
stdlib/public/stubs/LibcShims.cpp
Outdated
There was a problem hiding this comment.
Is a blocking read from the OS's entropy source the right behavior here? For applications like server side Swift we can pretty well expect a fast response from the OS because it'll have initialized its entropy sources by the time a Swift process is up. But for embedded platforms this call might take as long as minutes if the OS deems there's insufficient entropy.
There's pros and cons either way, I would just like to see a rationale for this somewhere for reference.
There was a problem hiding this comment.
The version of getrandom we're using here only blocks on OS startup where /dev/urandom may not have been initialized yet. After /dev/urandom is initialized, getrandom will always call into it, which makes this call non-blocking 99.99% of the time. The proposal explains this and shares that Python's implementation uses getrandom() as well, but falls back on /dev/urandom if getrandom() deems it needed to block on OS startup. We could do something similar.
By default, getrandom() draws entropy from the urandom source (i.e.,
the same source as the /dev/urandom device). This behavior can be
changed via the flags argument.
If the urandom source has not yet been initialized, then getrandom()
will block, unless GRND_NONBLOCK is specified in flags.
There was a problem hiding this comment.
The proposal explains this and shares that Python's implementation uses getrandom() as well, but falls back on /dev/urandom if getrandom() deems it needed to block on OS startup. We could do something similar.
We could, but doesn't Python 3.6 do what you've got here now?
There was a problem hiding this comment.
Ah you're right, 3.6 does what we have here and 3.5 fell back on reading /dev/urandom.
There was a problem hiding this comment.
Isn't this all just a very verbose spelling of:
let delta = Bound.Magnitude(truncatingIfNeeded: upperBound &- lowerBound)
% if 'Closed' in Range:
if delta == .max {
return Bound(truncatingIfNeeded: generator.next() as Bound.Magnitude)
}
delta += 1
% end
return lowerBound &+ Bound(truncatingIfNeeded: generator.next(upperBound: delta))
?
You're asking a lot of the optimizer here. Or am I missing something?
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
FWIW, in arc4random_uniform, we choose the smallest N such that 2^N is greater than or equal to upperBound, and then pull N bits at a time from the random source until we get a number smaller than upperBound. This avoids needing to compute two % operations, which is pretty significant.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Does FixedWidthInteger actually require that the representable range be 0 ..< 2^N for some N? It probably should, but I can't remember that it's actually spelled out anywhere, and this implementation assumes it.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
It would be nice to be able to just iterate over .words here and slam in bits from the source. Words is under-constrained though, which makes this a pain, I think.
There was a problem hiding this comment.
Adding initializer requirements like BinaryInteger.init<S: Sequence>(words: S) where S.Element == UInt or FixedWidthInteger.init<S>(truncatingIfNeeded words: S) would help, even if we can't fix Words. That's a separate proposal, though.
(What if we added a new protocol specifically for Words? If it wasn't related to Collection, it would sidestep the compiler performance issue.)
There was a problem hiding this comment.
This implementation is not quite right; I believe that it has a slight bias to numbers with even significands, will not generate some values when the range spans the origin, and will barf when upperBound - lowerBound overflows. I should be able to provide a fixed implementation sometime later this week.
stdlib/public/stubs/LibcShims.cpp
Outdated
There was a problem hiding this comment.
getentropy is usually used for seeding. zx_cprng_draw seems like the function we oughta call here but I'm no Googler so @zbowling?
|
Yeah, zx_cprng_draw is the lowest level way to get RNG on Fuchsia.
getentropy should also work as it calls zx_cprng_draw now and now libc++
std::random_device should have support for getentropy as a backend (including
our version in Fuchsia) from a patch we landed up stream. See:
https://reviews.llvm.org/D40319
zx_cprng_draw gives you more control when the entropy pool is empty than
the genentropy API does (very unlikely) but we generally prefer it in code
repos outside of our own use any standard APIs when possible today as our
APIs are subject to change (although getting more and more unlikely over
time).
Zac Bowling
…On Sun, May 6, 2018 at 6:55 PM, Robert Widmann ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In stdlib/public/stubs/LibcShims.cpp
<#16413 (comment)>:
> + result; \
+})
+
+SWIFT_RUNTIME_STDLIB_INTERNAL
+void swift::_stdlib_random(void *buf, __swift_size_t nbytes) {
+ while (nbytes > 0) {
+ __swift_ssize_t actual_nbytes = -1;
+#if defined(GRND_RANDOM)
+ static const bool getrandom_available =
+ !(getrandom(nullptr, 0, 0) == -1 && errno == ENOSYS);
+ if (getrandom_available) {
+ actual_nbytes = WHILE_EINTR(getrandom(buf, nbytes, 0));
+ }
+#elif defined(__Fuchsia__)
+ __swift_size_t getentropy_nbytes = std::min(nbytes, __swift_size_t{256});
+ if (0 == getentropy(buf, getentropy_nbytes)) {
getentropy is usually used for seeding. zx_cprng_draw
<https://fuchsia.googlesource.com/zircon/+/HEAD/docs/syscalls/cprng_draw.md>
seems like the function we oughta call here but I'm no Googler so
@zbowling <https://github.com/zbowling>?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16413 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPAfoti-WwwMFCJEpIwI5yLs44BOeGhks5tv6l7gaJpZM4TzgWL>
.
|
|
Unless there are any last-minute concerns, I'm going to merge this as it stands, and items from this PR can be resolved by follow-up commits. |
|
|
|
@swift-ci please test and merge |
|
@swift-ci Please test and merge |
|
@swift-ci please test macOS platform |
|
Build failed |
|
Hmm. Seems to be failing reproducibly on the simulator, but only specifically on the Double testing. |
|
I can look into it when I get home, although that won’t be for another 8 hours 😕 |
There was a problem hiding this comment.
Most of my comments aren't important enough to block this PR, except for the issue with the generic implementation of Random.next(). I'm requesting changes for that one issue only; although I'm also fine with landing this now and fixing that in a followup -- but it should be fixed soon.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Adding initializer requirements like BinaryInteger.init<S: Sequence>(words: S) where S.Element == UInt or FixedWidthInteger.init<S>(truncatingIfNeeded words: S) would help, even if we can't fix Words. That's a separate proposal, though.
(What if we added a new protocol specifically for Words? If it wasn't related to Collection, it would sidestep the compiler performance issue.)
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Masking is unnecessary here, and the entire remainder case can be rolled into the loop. (The &<< will shift off high bits that aren't representable.)
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Luckily, the static FixedWidthInteger.bitWidth property has the following note:
An unsigned, fixed-width integer type can represent values from
0through(2 ** bitWidth) - 1, where**is exponentiation.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
This should be internal @usableFromInline // @testable. The symbol will then become accessible in tests as long as they are done with %target-run-stdlib-swift instead of %target-run-simple-swift.
test/stdlib/Random.swift
Outdated
There was a problem hiding this comment.
We could do the same coverage test we do for Int8, by inserting the selected elements into a set.
We should also test how randomElement behaves on an empty collection.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
This method seems like a good candidate for an @effects(releasenone) attribute.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Is this right? The shim calls arc4random whenever __APPLE__ is defined.
There was a problem hiding this comment.
Ah good spot – the decision for the Apple implementation is to use one consistent implementation regardless of platform version. The caveats of OS version determining the method of generation exist today for arc4random and should extend to this implementation.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
I wonder how much resilience costs us for Random; Random.default.next() involves an indirectly-sized temporary and three resilient function calls, when we only really need a single call.
Do we expect that Random may become non-empty in the future? If not, we should make it @_fixed_layout, and we should make the initializer and .default's getter and setter inlinable.
There was a problem hiding this comment.
Is there a way to mark a private initializer as @inlinable or something similar? I can't use a private initializer in an @inlinable getter/setter for a var, but I can if I make the initializer internal with @usableFromInline. I wonder if we have to make this change in order to achieve @inlinable for the static var.
There was a problem hiding this comment.
If we're okay with making it inlinable, I think @inlinable internal is the way to do it.
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
Alas, this isn't right; FixedWidthInteger types aren't necessarily bitwise initializable like this.
It may very well be that we can't easily do better than to provide four additional concrete overloads for UInt8, UInt16, UInt32 and UInt, and leave the generic case to the default implementation. 😞
A more complicated way to resolve this would be to implement these next() overloads using a new (defaulted) _random(using:) requirement on FixedWidthInteger, which the standard fixed width integers could implement using _stdlib_random. To make this work, we should expose a customizable _fillBuffer operation on RandomNumberGenerator.
protocol RandomNumberGenerator {
mutating func next() -> UInt64
// FIXME: De-underscore after swift-evolution amendment
mutating func _fillBuffer(_ buffer: UnsafeMutableRawBufferPointer)
}
extension RandomNumberGenerator {
public mutating func _fillBuffer(_ buffer: UnsafeMutableRawBufferPointer) {
// Implementation in terms of next()
}
// FIXME: Remove after swift-evolution amendment
@inlinable
public mutating next<T: FixedWidthInteger & UnsignedInteger>() -> T {
return T._random(using: &self)
}
}
extension Random {
@effects(releasenone)
public mutating func _fillBuffer(_ buffer: UnsafeMutableRawBufferPointer) {
_stdlib_random(buffer)
}
}
protocol FixedWidthInteger: ... {
...
// Underscored to prevent misuse
static func _random<R: RandomNumberGenerator>(using generator: inout R) -> Self
}
extension FixedWidthInteger {
@inlinable
public static func _random<R: RandomNumberGenerator>(using generator: inout R) -> Self {
// Generic implementation with &<< and | (or +)
}
}
extension UInt8 {
@inlinable
public static func _random<R: RandomNumberGenerator>(using generator: inout R) -> UInt8 {
var result: UInt8 = 0
withUnsafeMutableBytes(of: &result) { generator._fillBuffer($0) }
return result
}
}
// etc. for the other standard fixed width integer types
stdlib/public/core/Random.swift
Outdated
There was a problem hiding this comment.
If filling a buffer with random bytes is the primitive operation around which we're building the default RNG, it should probably be exposed as a customizable public operation on RandomNumberGenerator. (I.e., I think it should be a requirement, with a default implementation in terms of the 64-bit next.)
(Update: I see you've already made the case for this on swift-evolution. 👍)
There was a problem hiding this comment.
Yeah I see that as a good future enhancement.
* Use the `__has_include` and `GRND_RANDOM` macros * Use `getentropy` instead of `getrandom` * Use `std::min` from the <algorithm> header * Move `#if` out of the `_stdlib_random` function * Use `getrandom` with "/dev/urandom" fallback * Use `#pragma comment` to import "Bcrypt.lib" * <https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp> * <https://clang.llvm.org/docs/UsersManual.html#microsoft-extensions> * Use "/dev/urandom" instead of `SecRandomCopyBytes` * Use `swift::StaticMutex` for shared "/dev/urandom" * Add `getrandom_available`; use `O_CLOEXEC` flag Add platform impl docs Update copyrights Fix docs Add _stdlib_random test Update _stdlib_random test Add missing & Notice about _stdlib_random Fix docs Guard on upperBound = 0 Test full range of 8 bit integers Remove some gyb Clean up integerRangeTest Remove FixedWidthInteger constraint Use arc4random universally Fix randomElement Constrain shuffle to RandomAccessCollection warning instead of error Move Apple's implementation Fix failing test on 32 bit systems
…eir raw memory Custom FixedWidthInteger types may not support this. Introduce a new (non-public) FixedWidthInteger requirement for generating random values; implement it using &<</+ in the generic case, and specialize it using RandomNumberGenerator._fill(bytes) for the builtin types.
| var tmp: Self = 0 | ||
| for i in 0 ..< quotient + remainder.signum() { | ||
| let next: UInt64 = generator.next() | ||
| tmp += Self(truncatingIfNeeded: next) &<< (UInt64.bitWidth * i) |
There was a problem hiding this comment.
It might be worth opening jira / radar to track making this less spectacularly inefficient for bignums at some future point?
There was a problem hiding this comment.
I think adding required BinaryInteger initializers to initialize integers from a given word or sequence of words would resolve this. I filed https://bugs.swift.org/browse/SR-7648; we should write up a swift-evolution pitch/proposal at some point. (cc @moiseev)
(The code above is defined on FixedWidthInteger, which uses a similar shifting loop for its own generic integer conversions. Frustratingly, FixedWidthInteger is currently impossible to implement using only public API: it requires an underscored version of the single-word initializer, without providing a default implementation.)
There was a problem hiding this comment.
Thanks for writing that up!
|
@Azoy By a (happy?) accident, I pushed two commits directly on your branch instead of my own fork; they fix the FixedWidthInteger issue I noticed. Provided the bots are in a cooperating mood, I want to re-do the tests to try reproducing the failure on the 32-bit Simulator -- I can't reproduce it myself with the latest changes. |
|
@lorentey I actually fixed those in my commit last night! |
|
@Azoy Nice! Unless anyone spots an issue with the last round of changes, LGTM! We just need to wait for the bots to come back. |
|
@lorentey Agree. Assuming tests pass, let's land this so people can start using it. We'll find the problems faster that way anyway =) |
|
@swift-ci please test |
|
(needs full test to get the simulators) |
xwu
left a comment
xwu
left a comment
There was a problem hiding this comment.
Sorry to be late here. Some initial comments.
| public static func random<T: RandomNumberGenerator>( | ||
| using generator: inout T | ||
| ) -> Bool { | ||
| return (generator.next() >> 17) & 1 == 0 |
There was a problem hiding this comment.
This isn't totally insane, because for (some) very weak RNGs middle bits have better randomness properties than low bits, but really no one should be using those ...
There was a problem hiding this comment.
Nate sent this in the initial pr:
For some random generators the low bits have higher levels of repetition than the higher bits do. Could we change this to (generator.next() >> 17) & 1 == 0?
There was a problem hiding this comment.
Got it; seemed strange because 17 is pretty arbitrary. If we want to use high bits, Int(bitPattern: generator.next()) < 0 would be less mystifying, IMO.
There was a problem hiding this comment.
@xwu you don't want the high bits either, those can also be weak. You really want some "middle" bits. This is worth a comment, though.
There was a problem hiding this comment.
Go ahead, try to name a number more random than 17.
There was a problem hiding this comment.
FWIW, according to some cursory testing, the GKRandomSource types use 31 for the offset in nextBool() (i.e., the high bit, like @xwu suggested).
| ) { | ||
| let count = self.count | ||
| guard count > 1 else { return } | ||
| var amount = count |
There was a problem hiding this comment.
Nit: we don't need both let count and var amount in this method, it seems.
| random = next() | ||
| } while random < range | ||
|
|
||
| return random % upperBound |
There was a problem hiding this comment.
I'm concerned about this implementation here. If % upperBound is necessary, then we have modulo bias; if not, then we should not have this operation here.
There was a problem hiding this comment.
This does not introduce modulo bias; it has already been accounted for discarding draws smaller than range. This is a standard technique of eliminating modulo bias while minimizing the expected number of bits that you need to draw from your source. It does require two non-constant modulus operations, so for most random sources it's preferable to use the other technique that I sketched out for performance reasons, but it is bias-free.
There was a problem hiding this comment.
Ah yes, I didn't study the preceding lines in great detail. Agree that there's room for performance improvement.
| /// The default instance of the `Random` random number generator. | ||
| public static var `default`: Random { | ||
| get { return Random() } | ||
| set { /* Discard */ } |
There was a problem hiding this comment.
Does Random have to be a struct? If it's a final class, then we don't have to make this a static var and the setter can be deleted. As it is, I worry that this is a code smell; should we at least fatalError if the user attempts to use the setter?
There was a problem hiding this comment.
The interface for using generators takes them inout to allow for value typed generators. This means you can't fatalError on the set because it will be called (if you put code in it... not if you don't hopefully :).
|
@swift-ci please test and merge |
|
@swift-ci Please test and merge |
|
@natecook1000 the failure looks unrelated, could you test again? |
|
@swift-ci please test macOS platform |
|
This looks ready to merge 🎉 |
11 participants
Alright, lets try this again!
This is the implementation for SE-0202