Skip to content

Minor simplification to FixedWidthInteger._random(using: &generator). #27056

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Minor simplification to FixedWidthInteger._random(using: &generator). #27056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcb670e
Minor simplification to `FixedWidthInteger._random(using: &generator)`.
stephentyrone Sep 6, 2019
ddd54aa
Remove a layer of indirection for random on Apple platforms.
stephentyrone Sep 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions stdlib/public/core/Integers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3365,19 +3365,9 @@ extension FixedWidthInteger {
public static func _random<R: RandomNumberGenerator>(
using generator: inout R
) -> Self {
if bitWidth <= UInt64.bitWidth {
return Self(truncatingIfNeeded: generator.next() as UInt64)
stride(from: 0, to: bitWidth, by: 64).reduce(into: .zero) {
$0 |= Self(truncatingIfNeeded: generator.next()) &<< $1
}

let (quotient, remainder) = bitWidth.quotientAndRemainder(
dividingBy: UInt64.bitWidth
)
var tmp: Self = 0
for i in 0 ..< quotient + remainder.signum() {
let next: UInt64 = generator.next()
tmp += Self(truncatingIfNeeded: next) &<< (UInt64.bitWidth * i)
}
return tmp
}
}

Expand Down
6 changes: 6 additions & 0 deletions stdlib/public/core/Random.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,9 @@ public struct SystemRandomNumberGenerator: RandomNumberGenerator {
return random
}
}

#if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
@_silgen_name("arc4random_buf")
@usableFromInline
internal func swift_stdlib_random(_: inout UInt64, _: Int)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
internal func swift_stdlib_random(_: inout UInt64, _: Int)
internal func swift_stdlib_random(_: UnsafeMutableRawPointer, _: Int)

The validation-test uses swift_stdlib_random with buffers larger than a UInt64.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also arc4random_buf uses the C calling conv.

Copy link
Contributor

@jrose-apple jrose-apple Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you can't use @_silgen_name to declare arbitrary C things regardless, because they'll collide with the real declarations at the SIL level when someone imports them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If swift_stdlib_random (for __APPLE__ platforms) was moved from Random.cpp to Random.h, using static inline SWIFT_ALWAYS_INLINE, would it be imported into Swift as @inlinable?

Copy link
Contributor Author

@stephentyrone stephentyrone Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we do this in a bunch of other places, and it's also (almost) what I'll do here. I'll update this sometime this week. (We can't quite just make the function static inline, because we need to still have an external symbol to support code compiled with an earlier version of Swift, but that's easy enough to address.)

Minor note: it's not semantically equivalent to @inlinable--it has the semantics of a C[++] always-inline function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your contention that we shouldn't have any such static inline wrappers in the shims? If so, what's the alternative--just eat the performance hit for all of these operations? For random there's enough work happening that it's not catastrophic, but it's still measurable, and for some of these other operations it's worse.

Copy link
Contributor

@jrose-apple jrose-apple Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule is that the static ones can't call anything declared elsewhere. We've been eating the perf hit where the indirection is used for several releases; we can take a little longer to come up with a proper solution (or figure out how to compile with LTO).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem is also specific to the standard library, because that's the only library that's below the $Platform module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule is that the static ones can't call anything declared elsewhere.

Except for strlen and memcmp (both of which actually have clang builtins, IIRC, so I think we could fix those) and malloc_size and free and ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I was tagged into #19614 and didn't complain then. I do not trust any of this though—the types are only matching up because the importer special-cases a few of the shims types like it does the real libc types, and the more we do it the more likely we are to accidentally diverge from the real SDK.

#endif