<experimental/random> Implement randint #361

SuperWig · 2019-12-09T11:46:07Z

Description

Implemented std::experimental::randint from library fundamentals v3.

It's very common for beginners to want random ints for implementing things like tic-tac-toe or a variety of other terminal based games when they're learning C++.
However rand() is discouraged (I believe STL said even beginners shouldn't be using it - or something to that effect on reddit before) and <random> is not beginner friendly so I believe this would be a nice addition.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

miscco

As a bystander this looks good. I had some comments though

stl/inc/experimental/random

miscco · 2019-12-09T12:45:40Z

stl/inc/experimental/random

+        }
+
+        inline void reseed() {
+            _Engine().seed(random_device{}());


This should be default_seed

Then that wouldn't satisfy "The first form sets g to an unpredictable state". Default seed is predictable.

Hm I took this from the actual random library. It might be that they differ but maybe a maintainer can shed some light here.

Looking at the specification you linked to it seems that you need to reset the uniform_int_Distribution. At least that is what the note says.

They do differ, the standard says seed() is equivalent to E() and that "Creates an engine with the same initial state as all other default-constructed engines of type E"

https://eel.is/c++draft/rand.req.eng#tab:rand.req.eng

Yeah, I concure with that. This leaves only the reset of the distribution

Maybe I'm missing something but Isn't that taken care of by the fact that a new instance of uniform_int_distribution is created whenever randint is called?

I think you are safe in the reseed case. However, in the usual case the result might differ, depending on whether the implementation of the distribution is statefull.

Imagine the simple case of int. The random_engine gives 64 bits of entropy. You only need 32 of those. So rather than throwing them away you can reuse them for the next call.

However, you are lucky as uniform_int_distribution is stateless

You only need 32 of those. So rather than throwing them away you can reuse them for the next call.

uniform_int_distribution can't do that because different calls to operator() may be passed different engines; it would be improper if the result depended on any engine other than that passed to operator().

SuperWig · 2019-12-09T13:21:31Z

As a bystander this looks good. I had some comments though

Thanks for the review. I'll tackle your comments later today unless the PR gets rejected before then.

miscco · 2019-12-09T15:30:45Z

stl/inc/experimental/random

+                              unsigned long, unsigned long long>,
+                "template argument does not meet the requirements for IntType.");
+            _STL_ASSERT(_Min <= _Max, "_Min > _Max");
+            return uniform_int_distribution<_IntType>{_Min, _Max}(_Engine());


This is not correct. You are creating a new uniform_int_distribution with a given range and Call operator() on that temporary. However, the specification speaks from a thread_local instance of a uniform_int_distribution. Now I fail to see how that is possible if there are different calls to randint with different parameters. @BillyONeal might know, at least i remember him talking about rngs.

In any case the initialization of the random number engines/distributions is generally rather costly.So recreating one seems Bad.

In any case the initialization of the random number engines/distributions is generally rather costly.So recreating one seems Bad.

AFAIK distributions aren't costly only engines. And the engine being used is a static thread_local (static is implied by thread_local unless I'm mistaken) instance that's returned by reference.

However, the specification speaks from a thread_local instance of a uniform_int_distribution.

That's not observable because uniform_int_distribution is stateless.

Specifically, in our current implementation, uniform_int_distribution is stateless (with respect to the random bits; of course the limits are stored), but the Standard permits random bits to be retained.

but the Standard permits random bits to be retained

It does? Huh, TIL. http://eel.is/c++draft/rand.req.dist#tab:rand.req.dist says there are only requirements on operator() if the same URBG is used successively.

http://eel.is/c++draft/rand.req.dist#tab:rand.req.dist says there are only requirements on operator() if the same URBG is used successively.

That's imposing the uniformity requirement. If you invoke a distribution repeatedly with different engines, then uniformity isn't guaranteed. Indeed it could not be, since I could invoke the distribution repeatedly with the same engine state.

The hint that distributions are allowed to cache some random bits is the specification (and existence) of reset, which guarantees that "Subsequent uses of d do not depend on values produced by any engine prior to invoking reset." (i.e., any cached bits are flushed).

stl/inc/experimental/random

BillyONeal

The thread_local issue might be a spec bug but there's a reason we've avoided implementing stuff not in the WP...

BillyONeal · 2019-12-09T22:18:00Z

stl/inc/experimental/random

+    inline namespace fundamentals_v3 {
+
+        inline default_random_engine& _Engine() {
+            thread_local default_random_engine _Eng{random_device{}()};


I don't think we can use thread_local in standard headers. I know the spec talks about this, but IMO this is one of the things we need to fix in the spec before it would be ready to actually merge.
I think we should return rand_s or _Random_device() (which calls rand_s) instead of replacing the platform's good and secure RNG with a poor linear congruential engine...

What's wrong with thread_local, if it's mandated by the TS?

rand_s() is not available within STL headers (as the CRT guards its declaration with a macro). _Random_device() is available, but I see no reason to avoid random_device as it's stateless and its default constructor no longer constructs a string.

default_random_engine is mt19937 for MSVC's STL:

STL/stl/inc/random

Line 4938 in 8f94319

using default_random_engine = mt19937;

We cannot replace the TS's use of default_random_engine with directly using random_device, as that breaks the functionality of reseed(value).

What's wrong with thread_local, if it's mandated by the TS?

The TS doesn't know that that causes a .TLS section to be emitted and that there are a limited number of those. If any .obj in kernel32.dll included the header, for example, that would critically break any program near the ~100 or so TLS slot limit.

We put other facilities that require thread-local like behavior in the vcruntime PTD structure, in part to avoid such mess. (And also to make fibers work)

mersenne_twister/default_random_engine are particularly problematic because they are huge, so a program that creates many threads which was just under hardware/implementation limits before might be over those limits after. In that sense, thread_local breaks the "don't pay for what you don't use" principle because most threads aren't going to use all thread_local data members.

rand_s() is not available within STL headers

Right, I'm referring to the facility (that being Windows default cryptographically secure random engine)

We cannot replace the TS's use of default_random_engine with directly using random_device, as that breaks the functionality of reseed(value).

Yeah, I wasn't sure if the reseed function was an artifact of the recommended implementation or a hard requirement driven by design goals. I asked on L[E]WG mailing lists and it was indicated there that the ability to reseed is indeed a design goal.

We should decide whether a created thread is supposed to derive its random state from the calling thread (like locale does) or if it should get a brand new state.

BillyONeal · 2019-12-09T22:18:40Z

stl/inc/experimental/random

+                              unsigned long, unsigned long long>,
+                "template argument does not meet the requirements for IntType.");
+            _STL_ASSERT(_Min <= _Max, "_Min > _Max");
+            return uniform_int_distribution<_IntType>{_Min, _Max}(_Engine());


However, the specification speaks from a thread_local instance of a uniform_int_distribution.

That's not observable because uniform_int_distribution is stateless.

zhihaoy · 2019-12-09T23:06:54Z

I want to mention that the randint facilities also include std::experimental::sample and std::experimental::shuffle.

StephanTLavavej · 2019-12-09T22:52:22Z

stl/inc/experimental/random

@@ -0,0 +1,50 @@
+// random experimental header
+


I am willing to make a special exception to our non-goal of "Implementing Technical Specifications" on a case-by-case basis for this feature, given how obnoxious the lack of easy random number generation is for beginners, but I am concerned about permanently increasing our support cost, and the lack of progress for this proposal within WG21.

stl/CMakeLists.txt

StephanTLavavej · 2019-12-09T23:11:54Z

stl/inc/experimental/random

+    inline namespace fundamentals_v3 {
+
+        inline default_random_engine& _Engine() {
+            thread_local default_random_engine _Eng{random_device{}()};


What's wrong with thread_local, if it's mandated by the TS?

rand_s() is not available within STL headers (as the CRT guards its declaration with a macro). _Random_device() is available, but I see no reason to avoid random_device as it's stateless and its default constructor no longer constructs a string.

default_random_engine is mt19937 for MSVC's STL:

STL/stl/inc/random

Line 4938 in 8f94319

using default_random_engine = mt19937;

We cannot replace the TS's use of default_random_engine with directly using random_device, as that breaks the functionality of reseed(value).

stl/inc/experimental/random

StephanTLavavej · 2019-12-09T23:33:23Z

stl/inc/experimental/random

+                              unsigned long, unsigned long long>,
+                "template argument does not meet the requirements for IntType.");
+            _STL_ASSERT(_Min <= _Max, "_Min > _Max");
+            return uniform_int_distribution<_IntType>{_Min, _Max}(_Engine());


Specifically, in our current implementation, uniform_int_distribution is stateless (with respect to the random bits; of course the limits are stored), but the Standard permits random bits to be retained.

stl/inc/experimental/random

stl/inc/experimental/algorithm

stl/inc/experimental/random

StephanTLavavej

After refactoring reseed() for use in engine initialization, I believe that this will be ready to merge (with test changes added in the Microsoft-internal repo; we are getting ever closer to making the tests public).

stl/inc/experimental/random

BillyONeal · 2019-12-12T04:25:21Z

After refactoring reseed() for use in engine initialization, I believe that this will be ready to merge (with test changes added in the Microsoft-internal repo; we are getting ever closer to making the tests public).

I think we still need the fiber resistance added that the rest of the "thread_local" ish state from the libraries provide. Ideally we would staple this to the vcruntime ptd somehow, but given how large a mersenne twister is and the relative unlikelihood of this actually making it into the standard maybe it's too early to commit to paying that cost on every thread and fiber forever.

As a result I remain weakly opposed to merging this feature unless we have some means of preventing users who don't use the experimental feature from paying costs related to the experimental feature (thread_local makes everyone pay that cost, whether they touch this feature or not).

BillyONeal · 2019-12-12T04:26:49Z

I just realized that this might also be triggering the DllMain / loader lock monster. Won't this try to call RtlGenRandom (via rand_s, via _Random_device, via random_device::operator()) under the loader lock? That's not safe.

MikeGitb · 2019-12-12T07:28:51Z

Regarding the size: If you want to use TLS at ~~ADL~~ all, I think this would be a perfectly valid case for allocating the engine on the heap and only storing a ptr to it on the TLS.

StephanTLavavej · 2019-12-12T08:04:15Z

[@BillyONeal]

I think we still need the fiber resistance added that the rest of the "thread_local" ish state from the libraries provide.

I don't understand fibers, so I defer to your judgement.

some means of preventing users who don't use the experimental feature from paying costs related to the experimental feature (thread_local makes everyone pay that cost, whether they touch this feature or not).

<experimental/algorithm> and <experimental/random> are header-only and leaf headers, so it's weakly "you don't pay for what you don't include". But that's a valid reason to be opposed.

I just realized that this might also be triggering the DllMain / loader lock monster. Won't this try to call RtlGenRandom (via rand_s, via _Random_device, via random_device::operator()) under the loader lock? That's not safe.

I am definitely very concerned about any potential loader lock interaction with thread_local initialization. We should probably ask a compiler dev if we want to merge this implementation.

[@MikeGitb]

Regarding the size: If you want to use TLS at ADL, I think this would be a perfectly valid case for allocating the engine on the heap and only storing a ptr to it on the TLS.

That's slightly observable (new could be replaced with an instrumented version), but that is a potential workaround.

MikeGitb · 2019-12-12T09:08:40Z

That's slightly observable (new could be replaced with an instrumented version), but that is a potential workaround.

Is the stdlib not allowed to create hidden calls to new?

This reverts commit dc7c56f.

BillyONeal · 2019-12-13T06:02:10Z

That's slightly observable (new could be replaced with an instrumented version), but that is a potential workaround.

Is the stdlib not allowed to create hidden calls to new?

We're allowed to but try to not unless the standard explicitly tells us to do so.

Regarding the fibers stuff, that would mean attaching it to an FLS slot just like we do elsewhere; dig around in your Visual Studio sources and you should see e.g. __acrt_get_ptd, __vcrt_get_ptd etc. "ptd" stands for "per thread data" but it's actually per fiber data in modern versions of the CRT. The "acrt" ones are part of ucrtbase.dll (part of Windows). e.g. on this system I'm talking about these:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\VC\Tools\MSVC\14.24.28314\crt\src\vcruntime\dll_dllmain.cpp
C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\VC\Tools\MSVC\14.24.28314\crt\src\vcruntime\per_thread_data.cpp

To do this randint feature really correctly we would probably want to copy exactly what that ptd stuff is doing there, and put it in a new satellite DLL so that the state of reseed is shared correctly across DLL boundaries just like other shared standard library state (e.g. default_memory_resource).

I talked with the designers of this feature and while they strongly care about reseed, they do not strongly care about the actual engine used being default_random_engine. They filed a new defect report: LWG-3357. As a result we might want to ask @imneme and contributors if they'd be willing to add the LLVM exception and we'd use PCG, which has better distribution characteristics than mersenne twister and also doesn't need a huge state.

StephanTLavavej · 2020-01-25T03:08:12Z

Thanks again for working on this and going through our lengthy review process. Now that we're back from the holidays and catching up on our PR backlog, we've discussed what to do about randint.

We've decided against merging this PR, due to our concerns with how the feature as currently specified in the TS interacts with our platform. There's too much risk (and support cost) to justify making an exception to our usual Non-Goals policy regarding implementing TS features.

As mentioned above, we're concerned about:

Interactions with our per thread/fiber data and DLLs
Ensuring that loader lock problems don't happen while initializing the engine's state
The size of Mersenne Twister potentially causing issues

All of this may be solvable, but this is much more work than initially expected.

Personally, I'd like to see randint refined for eventual Standardization, and I would also like to see PCG's high-quality generators proposed for Standardization (ideally with a reference implementation that's Apache+LLVM licensed so we can directly use it), which would resolve Mersenne Twister size concerns. Then I think we could figure out the PTD and initialization issues.

SuperWig · 2020-01-25T10:10:42Z

A bit disappointing but completely understandable. Thanks for taking the time to review it :)

<experimental/random> Implement randint

451bcc5

SuperWig requested a review from a team as a code owner December 9, 2019 11:46

Fix validation

7fcdf93

miscco reviewed Dec 9, 2019

View reviewed changes

SuperWig added 2 commits December 9, 2019 16:00

Improve assert message

51d4641

const arguments and use default argument for reseed()

2c268b5

miscco reviewed Dec 9, 2019

View reviewed changes

stl/inc/experimental/random Outdated Show resolved Hide resolved

Use _RNG_REQUIRE_INTTYPE for type requirment.

a452143

BillyONeal suggested changes Dec 9, 2019

View reviewed changes

StephanTLavavej requested changes Dec 9, 2019

View reviewed changes

SuperWig added 6 commits December 10, 2019 12:04

Add header to __msvc_all_public_headers.hpp

b9cc3cd

Add _NODISCARD

4fa838f

split reseed overloads and make reseed() initialise whole state

408a390

_STD qualify function calls

ecef960

Use braces for _Seq

dc7c56f

Implement <experimental/algorithm>

27a1b20

StephanTLavavej requested changes Dec 11, 2019

View reviewed changes

SuperWig added 3 commits December 11, 2019 12:52

Naming and formatting

a0231a7

Include <algorithm>

3c5b1a5

static_assert default_random_engine == mt19937

aa6bd67

StephanTLavavej requested changes Dec 12, 2019

View reviewed changes

stl/inc/experimental/random Outdated Show resolved Hide resolved

StephanTLavavej self-assigned this Dec 12, 2019

SuperWig added 2 commits December 12, 2019 17:35

Revert "Use braces for _Seq"

5ffe8bf

This reverts commit dc7c56f.

Properly seed _Engine()

58b5c96

certik mentioned this pull request Dec 19, 2019

Implement loadtxt and savetxt fortran-lang/stdlib#23

Merged

StephanTLavavej removed their assignment Jan 22, 2020

StephanTLavavej closed this Jan 25, 2020

SuperWig deleted the exp_random branch January 25, 2020 10:10

StephanTLavavej mentioned this pull request May 19, 2020

Library Fundamentals TS #846

Closed

<experimental/random> Implement randint #361

<experimental/random> Implement randint #361

Uh oh!

Conversation

SuperWig commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

miscco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperWig Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperWig Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miscco Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperWig commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyONeal Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhihaoy commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperWig commented Dec 9, 2019 •

edited

Loading

miscco left a comment •

edited

Loading

SuperWig Dec 9, 2019 •

edited

Loading

SuperWig Dec 9, 2019 •

edited

Loading

miscco Dec 9, 2019 •

edited

Loading

BillyONeal Dec 10, 2019 •

edited

Loading

MikeGitb commented Dec 12, 2019 •

edited

Loading