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

Add initRand() with seed based on time #16953

Merged
merged 3 commits into from
Feb 8, 2021
Merged

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Feb 6, 2021

I assumed there was a reason for this not existing, but now I'm not sure. This proc saves a times import and possible incorrect implementations from Rand initializations that want to use the current time, but don't want to use the global random state. Is there a reason this only exists for randomize?

The randomize with default time seed isn't tested, so I'm assuming this can't be tested either. Will add changelog entry after possible changes are considered

future work

EDIT(timotheecour)

  • use monotimes.getMonoTime instead, which has higher resolution.
    Otherwise, low res means lower quality initRand() (esp for multiple threads which would have their numbers more correlated, etc)
  • remove dependency on times (a heavy dependency) and use system/timers directly after we address system/timers and std/monotimes are mostly duplicated, should be refactored timotheecour/Nim#572
  • std/oids also needs to be updated, ideally calling common refactored code (possibly via some std/private/x if needed) in 2 places:
let t = getTime().toUnix.int32
...
var time = getTime().toUnix.int32
  • after this is fixed, we can add tests for initRand (it fails currently because of poor resolution):
  block: # initRand
    for i in 0..<100:
      var r1 = initRand()
      var r2 = initRand()
      doAssert r1.rand(0..high(int)) != r2.rand(0..high(int)), $i

links

Process ID, thread ID and system tick

##
## The resulting state is independent of the default random number generator's state.
##
## **Note:** Does not work for NimScript or the compile-time VM.
Copy link
Member

Choose a reason for hiding this comment

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

how about instead honor experimental:vmops:
if experimental:vmops is passed, it would use it, else would use a fixed seed (ie reproducible builds)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well you wouldn't use the random module if you cared about predictability. Current compilation isn't pure either, IOEffect procs already exist in VM (though not tagged as such), this would just use a TimeEffect proc. There's no way to check for that experimental, compileOption doesn't work for experimentals, compiles(getTime()) is true under when nimvm even if it doesn't work. Maybe it should be a compile option like --pureVM or --impureVM, but even then the entire random module wouldn't make sense as being pure, other languages have things like RandomEffect (Idris and I believe some Haskell libraries).

Copy link
Member

Choose a reason for hiding this comment

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

There's no way to check for that experimental

refs #8644 which is very fixable

Copy link
Member

@timotheecour timotheecour Feb 6, 2021

Choose a reason for hiding this comment

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

anyways, this can be fixed in future PR, just leave this comment as un-resolved

Copy link
Member

@timotheecour timotheecour left a comment

Choose a reason for hiding this comment

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

LGTM, all the other things I mentioned (including future work i added on top) can be done in followup PR's

@timotheecour timotheecour merged commit 4fac8af into nim-lang:devel Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants