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
[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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// This can be found at: stdlib/public/core/Random.swift | ||
|
||
#if defined(_WIN32) && !defined(__CYGWIN__) | ||
#error TODO: Test _stdlib_random on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to break the build for Win32 platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no, this does break builds if they're compiling the stdlib. I can probably substitute this in with a warning directive instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @compnerd
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just some minor comments.
I'll let @moiseev review the floating point and integer code.
stdlib/public/core/Random.swift
Outdated
if T.bitWidth <= UInt64.bitWidth { | ||
return T(truncatingIfNeeded: self.next()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luckily, the static FixedWidthInteger.bitWidth
property has the following note:
An unsigned, fixed-width integer type can represent values from
0
through(2 ** bitWidth) - 1
, where**
is exponentiation.
stdlib/public/core/Random.swift
Outdated
} while random < range | ||
|
||
return random % upperBound | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining the implementation strategy and the reason you chose it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// This can be found at: stdlib/public/core/Random.swift | ||
|
||
#if defined(_WIN32) && !defined(__CYGWIN__) | ||
#error TODO: Test _stdlib_random on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @compnerd
stdlib/public/stubs/LibcShims.cpp
Outdated
actual_nbytes = nbytes; | ||
#endif | ||
if (actual_nbytes == -1) { | ||
static const int fd = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -11,34 +11,44 @@ | |||
//===----------------------------------------------------------------------===// | |||
|
|||
#if defined(__APPLE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please re-outdent the conditional preprocessor directives.
static const bool getrandom_available = | ||
!(getrandom(nullptr, 0, 0) == -1 && errno == ENOSYS); | ||
if (getrandom_available) { | ||
actual_nbytes = WHILE_EINTR(getrandom(buf, nbytes, 0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you're right, 3.6 does what we have here and 3.5 fell back on reading /dev/urandom
.
) -> Element? { | ||
guard !isEmpty else { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} while random < range | ||
|
||
return random % upperBound | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if T.bitWidth <= UInt64.bitWidth { | ||
return T(truncatingIfNeeded: self.next()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
return T(truncatingIfNeeded: self.next()) | ||
} | ||
|
||
let (quotient, remainder) = T.bitWidth.quotientAndRemainder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
public static func random<T: RandomNumberGenerator>( | ||
in range: ${Range}<Self>, | ||
using generator: inout T | ||
) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
#elif defined(__Fuchsia__) | ||
__swift_size_t getentropy_nbytes = std::min(nbytes, __swift_size_t{256}); | ||
if (0 == getentropy(buf, getentropy_nbytes)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
return T(truncatingIfNeeded: self.next()) | ||
} | ||
|
||
let (quotient, remainder) = T.bitWidth.quotientAndRemainder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
if remainder != 0 { | ||
let random = next() | ||
let mask = UInt64.max &>> (UInt64.bitWidth - remainder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if T.bitWidth <= UInt64.bitWidth { | ||
return T(truncatingIfNeeded: self.next()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Luckily, the static FixedWidthInteger.bitWidth
property has the following note:
An unsigned, fixed-width integer type can represent values from
0
through(2 ** bitWidth) - 1
, where**
is exponentiation.
stdlib/public/core/Random.swift
Outdated
} | ||
} | ||
|
||
public // @testable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
let randomGreeting = greetings.randomElement() | ||
expectNotNil(randomGreeting) | ||
expectTrue(greetings.contains(randomGreeting!)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
/// Returns a value from a uniform, independent distribution of binary data. | ||
/// | ||
/// - Returns: An unsigned 64-bit random value. | ||
public mutating func next() -> UInt64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method seems like a good candidate for an @effects(releasenone)
attribute.
stdlib/public/core/Random.swift
Outdated
/// =================================== | ||
/// | ||
/// - Apple platforms use `arc4random_buf(3)` for newer versions of their OS, | ||
/// and for older versions they will read from `/dev/urandom`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? The shim calls arc4random whenever __APPLE__
is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// `/dev/urandom`. | ||
/// - `Fuchsia` calls `getentropy(3)`. | ||
/// - `Windows` calls `BCryptGenRandom`. | ||
public struct Random : RandomNumberGenerator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're okay with making it inlinable, I think @inlinable internal
is the way to do it.
stdlib/public/core/Random.swift
Outdated
/// every value of `T` is equally likely to be returned. | ||
public mutating func next<T: FixedWidthInteger & UnsignedInteger>() -> T { | ||
var random: T = 0 | ||
_stdlib_random(&random, MemoryLayout<T>.size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
|
||
public // @testable | ||
func _stdlib_random(_ bytes: UnsafeMutableRawBufferPointer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why >> 17
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead, try to name a number more random than 17
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🎉 |
Alright, lets try this again!
This is the implementation for SE-0202