Skip to content
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

Fix initrand to avoid random number sequences overlapping #18744

Merged
merged 11 commits into from
Sep 2, 2021
80 changes: 69 additions & 11 deletions lib/pure/random.nim
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,15 @@ when defined(js):
a0: 0x69B4C98Cu32,
a1: 0xFED1DD30u32) # global for backwards compatibility
else:
# racy for multi-threading but good enough for now:
var state = Rand(
const DefaultRandSeed = Rand(
a0: 0x69B4C98CB8530805u64,
a1: 0xFED1DD3004688D67CAu64) # global for backwards compatibility
a1: 0xFED1DD3004688D67CAu64)

# racy for multi-threading but good enough for now:
var state = DefaultRandSeed # global for backwards compatibility

func isValid(r: Rand): bool {.inline.} =
not (r.a0 == 0 and r.a1 == 0)

since (1, 5):
template randState*(): untyped =
Expand Down Expand Up @@ -617,15 +622,28 @@ proc shuffle*[T](x: var openArray[T]) =

shuffle(state, x)

when not defined(nimscript) and not defined(standalone):
import times
when not defined(standalone):
when defined(js):
import std/times
else:
when defined(nimscript):
import std/hashes
else:
import std/[hashes, os, sysrand, monotimes]

when compileOption("threads"):
import locks
var baseSeedLock: Lock
baseSeedLock.initLock

var baseState: Rand

proc initRand(): Rand =
## Initializes a new Rand state with a seed based on the current time.
## Initializes a new Rand state.
##
## The resulting state is independent of the default RNG's state.
##
## **Note:** Does not work for NimScript or the compile-time VM.
## **Note:** Does not work for the compile-time VM.
##
## See also:
## * `initRand proc<#initRand,int64>`_ that accepts a seed for a new Rand state
Expand All @@ -635,20 +653,60 @@ when not defined(nimscript) and not defined(standalone):
let time = int64(times.epochTime() * 1000) and 0x7fff_ffff
result = initRand(time)
else:
let now = times.getTime()
result = initRand(convert(Seconds, Nanoseconds, now.toUnix) + now.nanosecond)
proc getRandomState(): Rand =
Copy link
Member

Choose a reason for hiding this comment

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

move this nested proc to top-level scope, simpler/cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why moving nested proc to top-level scope makes simpler/cleaner?
It doesn't reduce a total number of lines of random module.
getRandomState proc is used only in initRand().

when defined(nimscript):
Araq marked this conversation as resolved.
Show resolved Hide resolved
result = Rand(
a0: CompileTime.hash.Ui,
a1: CompileDate.hash.Ui)
if not result.isValid:
result = DefaultRandSeed
else:
var urand: array[sizeof(Rand), byte]

for i in 0 .. 7:
if sysrand.urandom(urand):
copyMem(result.addr, urand[0].addr, sizeof(Rand))
if result.isValid:
break

if not result.isValid:
# When 2 processes executed at same time on different machines,
# `result` can still have same value.
let
pid = getCurrentProcessId()
t = getMonoTime().ticks
demotomohiro marked this conversation as resolved.
Show resolved Hide resolved
result.a0 = pid.hash().Ui
result.a1 = t.hash().Ui
if not result.isValid:
result.a0 = pid.Ui
result.a1 = t.Ui
demotomohiro marked this conversation as resolved.
Show resolved Hide resolved

if not result.isValid:
result = DefaultRandSeed

when compileOption("threads"):
baseSeedLock.withLock:
if not baseState.isValid:
baseState = getRandomState()
result = baseState
baseState.skipRandomNumbers
else:
if not baseState.isValid:
baseState = getRandomState()
result = baseState
baseState.skipRandomNumbers

since (1, 5, 1):
export initRand

proc randomize*() {.benign.} =
## Initializes the default random number generator with a seed based on
## the current time.
## random number source.
##
## This proc only needs to be called once, and it should be called before
## the first usage of procs from this module that use the default RNG.
##
## **Note:** Does not work for NimScript or the compile-time VM.
## **Note:** Does not work for the compile-time VM.
##
## **See also:**
## * `randomize proc<#randomize,int64>`_ that accepts a seed
Expand Down
7 changes: 2 additions & 5 deletions lib/std/tempfiles.nim
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ See also:
* `mkstemp` (posix), refs https://man7.org/linux/man-pages/man3/mkstemp.3.html
]#

import os, random, std/monotimes
import os, random


const
Expand Down Expand Up @@ -107,11 +107,8 @@ var nimTempPathState {.threadvar.}: NimTempPathState
template randomPathName(length: Natural): string =
var res = newString(length)
if not nimTempPathState.isInit:
var time = getMonoTime().ticks
demotomohiro marked this conversation as resolved.
Show resolved Hide resolved
when compileOption("threads"):
time = time xor int64(getThreadId())
nimTempPathState.isInit = true
nimTempPathState.state = initRand(time)
nimTempPathState.state = initRand()

for i in 0 ..< length:
res[i] = nimTempPathState.state.sample(letters)
Expand Down
23 changes: 23 additions & 0 deletions tests/stdlib/trandom.nim
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,26 @@ block: # bug #17670
type UInt48 = range[0'u64..2'u64^48-1]
let x = rand(UInt48)
doAssert x is UInt48

block: # bug #17898
# Checks whether `initRand()` generates unique states.
# size should be 2^64, but we don't have time and space.

# Disable this test for js until js gets proper skipRandomNumbers.
when not defined(js):
const size = 1000
var
rands: array[size, Rand]
randSet: HashSet[Rand]
for i in 0..<size:
rands[i] = initRand()
randSet.incl rands[i]

doAssert randSet.len == size

# Checks random number sequences overlapping.
const numRepeat = 100
for i in 0..<size:
for j in 0..<numRepeat:
discard rands[i].next
doAssert rands[i] notin randSet
Copy link
Member

Choose a reason for hiding this comment

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

Should be addressed in a follow up PR but the notion that a random sequence of numbers doesn't "overlap" with some other sequence seems to weaken the "randomness" and not to improve it.

Copy link
Member

Choose a reason for hiding this comment

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

i don't think think this is correct; the sequences of numbers generated can have overlap in parts, eg:
1 (9 8 3) 2 5
2 (9 8 3) 7 1

but the underlying states won't overlap (so long we remain within a period); if they did, the sequences generated would be identical at any point after which the state overlaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you tell me why is it weaken randomness?
It just works like evenly cut a random number sequence generated by calling next(r: var Rand) with single Rand, and assign each sub sequence to each Rand.
If you think randomness of random module is not good enough, I think we need to use different algorithm like xoroshiro128++, xoroshiro128** or PCG.

12 changes: 12 additions & 0 deletions tests/stdlib/trandom.nims
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import std/[random, sets]
demotomohiro marked this conversation as resolved.
Show resolved Hide resolved

block: # bug #17898
const size = 1000
var
rands: array[size, Rand]
randSet: HashSet[Rand]
for i in 0..<size:
rands[i] = initRand()
randSet.incl rands[i]

doAssert randSet.len == size