-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
initRand now uses strict monotonic counter to guarantee uniqueness #18149
Closed
+190
−32
Closed
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b8d04c2
refs #17898
timotheecour 2476060
_
timotheecour e7c3591
fix #17898 initRand now uses monotonic time to guarantee uniqueness
timotheecour 2fc0f9c
workaround refs #18158
timotheecour 7b314b7
improve tests
timotheecour 2d80e5c
fixup [skip ci]
timotheecour 9df2889
address comment
timotheecour 169f90c
add std/cputicks
timotheecour fbeff78
changelog
timotheecour fa6bf6c
add test for initRand in VM
timotheecour de27fc6
fixup
timotheecour File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
##[ | ||
Experimental API, subject to change | ||
]## | ||
|
||
#[ | ||
Future work: | ||
* convert ticks to time; see some approaches here: https://quick-bench.com/q/WcbqUWBCoNBJvCP4n8h3kYfZDXU | ||
* provide feature detection to test whether the CPU supports it (on linux, via /proc/cpuinfo) | ||
|
||
## further links | ||
* https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/ia-32-ia-64-benchmark-code-execution-paper.pdf | ||
* https://gist.github.com/savanovich/f07eda9dba9300eb9ccf | ||
* https://developers.redhat.com/blog/2016/03/11/practical-micro-benchmarking-with-ltrace-and-sched# | ||
]# | ||
|
||
when defined(js): | ||
proc getCpuTicksImpl(): int64 = | ||
## Returns ticks in nanoseconds. | ||
# xxx consider returning JsBigInt instead of float | ||
when defined(nodejs): | ||
{.emit: """ | ||
let process = require('process'); | ||
`result` = Number(process.hrtime.bigint()); | ||
""".} | ||
else: | ||
proc jsNow(): int64 {.importjs: "window.performance.now()".} | ||
result = jsNow() * 1_000_000 | ||
else: | ||
const header = | ||
when defined(posix): "<x86intrin.h>" | ||
else: "<intrin.h>" | ||
proc getCpuTicksImpl(): uint64 {.importc: "__rdtsc", header: header.} | ||
|
||
template getCpuTicks*(): int64 = | ||
## Returns number of CPU ticks as given by `RDTSC` instruction. | ||
## Unlike `std/monotimes.ticks`, this gives a strictly monotonic counter | ||
## and has higher resolution and lower overhead, | ||
## allowing to measure individual instructions (corresponding to time offsets in | ||
## the nanosecond range). | ||
## | ||
## Note that the CPU may reorder instructions. | ||
runnableExamples: | ||
for i in 0..<100: | ||
let t1 = getCpuTicks() | ||
# code to benchmark can go here | ||
let t2 = getCpuTicks() | ||
assert t2 > t1 | ||
cast[int64](getCpuTicksImpl()) | ||
|
||
template toInt64(a, b): untyped = | ||
cast[int64](cast[uint64](a) or (cast[uint64](d) shl 32)) | ||
|
||
proc getCpuTicksStart*(): int64 {.inline.} = | ||
## Variant of `getCpuTicks` which uses the `RDTSCP` instruction. Compared to | ||
## `getCpuTicks`, this avoids introducing noise in the measurements caused by | ||
## CPU instruction reordering, and can result in more deterministic results, | ||
## at the expense of extra overhead and requiring asymetric start/stop APIs. | ||
runnableExamples: | ||
var a = 0 | ||
for i in 0..<100: | ||
let t1 = getCpuTicksStart() | ||
# code to benchmark can go here | ||
let t2 = getCpuTicksEnd() | ||
assert t2 > t1, $(t1, t2) | ||
when nimvm: result = getCpuTicks() | ||
else: | ||
when defined(js): result = getCpuTicks() | ||
else: | ||
var a {.noinit.}: cuint | ||
var d {.noinit.}: cuint | ||
# See https://developers.redhat.com/blog/2016/03/11/practical-micro-benchmarking-with-ltrace-and-sched | ||
{.emit:""" | ||
asm volatile("cpuid" ::: "%rax", "%rbx", "%rcx", "%rdx"); | ||
asm volatile("rdtsc" : "=a" (a), "=d" (d)); | ||
""".} | ||
result = toInt64(a, b) | ||
|
||
proc getCpuTicksEnd*(): int64 {.inline.} = | ||
## See `getCpuTicksStart`. | ||
when nimvm: result = getCpuTicks() | ||
else: | ||
when defined(js): result = getCpuTicks() | ||
else: | ||
var a {.noinit.}: cuint | ||
var d {.noinit.}: cuint | ||
{.emit:""" | ||
asm volatile("rdtscp" : "=a" (a), "=d" (d)); | ||
asm volatile("cpuid" ::: "%rax", "%rbx", "%rcx", "%rdx"); | ||
""".} | ||
result = toInt64(a, b) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
discard """ | ||
targets: "c cpp js" | ||
matrix: "; -d:danger" | ||
""" | ||
|
||
import std/cputicks | ||
|
||
template main = | ||
let n = 100 | ||
for i in 0..<n: | ||
let t1 = getCpuTicks() | ||
let t2 = getCpuTicks() | ||
doAssert t2 > t1 | ||
|
||
for i in 0..<100: | ||
let t1 = getCpuTicksStart() | ||
# code to benchmark can go here | ||
let t2 = getCpuTicksEnd() | ||
doAssert t2 > t1 | ||
|
||
static: main() | ||
main() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 constrains the random module to work only on platforms where high-resolution tick counters are available - given that cputicks depends on non-standard behavior, it severly limits the platforms where Nim can be used - how to init
rand
is not a performance-critical operation - in fact, it would be trivial to continue using standardised C API for this without any significant loss -random
is not a cryptographic random source, it's a best-effort proposition upon which no code that actually requires randomness should rely upon - the granularity doesn't not change the utility of the module for any use cases for which its use is appropriate.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 doesn't; fallback code can always be added to
getCpuTicks
to return something similar to what std/monotimes returns. rdtsc is available on all x86 processors since the pentium, and other platforms that nim supports have equivalent instructions which can be wrapped bygetCpuTicks
, see google/benchmark code here https://github.com/google/benchmark/blob/v1.1.0/src/cycleclock.h#L116 which handles more platform than nim supports.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.
Do you mean it will be implemented in the future? Then change the
random.nim
after they are implement I think. Anyway let's wait for #18743.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.
see also nim-lang/RFCs#414 which would allow testing from presence of
__rdtsc
programmatically, at CT