Fix cong implementation to be properly random and not just cycling. #55509
Conversation
| if (max < 2) | ||
| return 0; |
There was a problem hiding this comment.
This seems biased against returning 1
There was a problem hiding this comment.
The old rng is also broken here, which is fun. But basically the issue is that it's a [0, n) interval. So to get 0 and 1 you pass in 2.
There was a problem hiding this comment.
Though I'll see if anyone that's using this is actually checking that.
…pen interval and not a closed one
|
@d-netto could you check the change I made to the GC code there. It looked to me that it was assuming that cong return 0 to max in a closed interval and that's not exactly correct. |
|
What matters here is that we want to generate a random number in 0, 1, ... markthreads - 1. It seems like the previous code was generating random numbers in [0, max], so that's why we have the markthreads - 1 in there. If the new implementation is generating random numbers in [0, max) then your change is fine. |
|
The old code was also open :) |
|
What about the: do {...} while (x > max)? |
|
Ah OK, there is a --max in there. |
|
The first line in the old implementation does max--. I think when I ported it from blogpost it came from it was there and wrong. |
gbaraldi
added
the
status:merge me
oscardssmith
added
domain:multithreading
|
I believe we should backport this as the consequences of the scheduler being non-random seems worse than the scheduler being a little slower? |
IanButterworth
added
backport 1.10
|
Do we have actual examples where the non-randomness causes things to break? |
|
Something along these lines? Maybe there's more to attribute the difference to though % julia +1.10.5 -t6
julia> foo() = @sync for _ in 1:Threads.nthreads()
Threads.@spawn for _ in 1:100000
yield()
end
end
foo (generic function with 1 method)
julia> using Chairmarks
julia> @b foo()
466.335 ms (33 allocs: 3.234 KiB, without a warmup)% julia +nightly -t6
julia> versioninfo()
Julia Version 1.12.0-DEV.1122
Commit 1e217279439 (2024-08-30 13:37 UTC)
...
julia> @b foo()
408.224 ms (33 allocs: 2.016 KiB, without a warmup) |
…55509) This was found by @IanButterworth. It unfortunately has a small performance regression due to actually using all the rng bits
Backported PRs: - [x] #55480 <!-- Fix push! for OffsetVectors, add tests for push! and append! on AbstractVector --> - [x] #55443 <!-- Add test for upper/lower/titlecase and fix call --> - [x] #55524 <!-- Set `.jl` sources as read-only during installation --> - [x] #55500 <!-- make jl_thread_suspend_and_get_state safe --> - [x] #55506 <!-- Fix indexing in _mapreducedim for OffsetArrays --> - [x] #55564 <!-- Empty out loaded_precompiles dict instead of asserting it's empty. --> - [x] #55567 <!-- Initialize threadpools correctly during sysimg build --> - [x] #55596 <!-- Fast bounds-check for CartesianIndex ranges --> - [x] #55605 <!-- Reroute Symmetric/Hermitian + Diagonal through triangular --> - [x] #55640 <!-- win: move stack_overflow_warning to the backtrace fiber --> - [x] #55715 <!-- Add precompile signatures to Markdown to reduce latency. --> - [x] #55593 <!-- Fix invalidations for FileIO --> - [x] #55555 <!-- Revert "Don't expose guard pages to malloc_stack API consumers" --> - [x] #55720 <!-- Fix `pkgdir` for extensions --> - [x] #55729 <!-- Avoid confounding compilation side effects of `@time_imports` --> - [x] #55718 <!-- Fix `@time_imports` extension recognition --> - [x] #55522 <!-- Fix tr for Symmetric/Hermitian block matrices --> Contains multiple commits, manual intervention needed: - [ ] #55509 <!-- Fix cong implementation to be properly random and not just cycling. --> Non-merged PRs with backport label: - [ ] #55641 <!-- fall back to slower stat filesize if optimized filesize fails --> - [ ] #55534 <!-- Set stdlib sources as read-only during installation --> - [ ] #55499 <!-- propagate the terminal's `displaysize` to the `IOContext` used by the REPL --> - [ ] #55458 <!-- Allow for generically extracting unannotated string --> - [ ] #55457 <!-- Make AnnotateChar equality consider annotations --> - [ ] #55453 <!-- Privatise the annotations API, for StyledStrings --> - [ ] #55355 <!-- relocation: account for trailing path separator in depot paths --> - [ ] #55220 <!-- `isfile_casesensitive` fixes on Windows --> - [ ] #55169 <!-- `propertynames` for SVD respects private argument --> - [ ] #54457 <!-- Make `String(::Memory)` copy --> - [ ] #53957 <!-- tweak how filtering is done for what packages should be precompiled --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia --> - [ ] #50813 <!-- More doctests for Sockets and capitalization fix --> - [ ] #50157 <!-- improve docs for `@inbounds` and `Base.@propagate_inbounds` --> - [ ] #41244 <!-- Fix shell `cd` error when working dir has been deleted -->
This was found by @IanButterworth. It unfortunately has a small performance regression due to actually using all the rng bits