Skip to content

Commit d04631d

Browse files
nsajkoKristofferC
authored andcommitted
don't mutate globals when constructing Rational from AbstractIrrational (#55853)
Relying on `ScopedValues`, set `BigFloat` precision without mutating the global default, while constructing `Rational` from `AbstractIrrational`. Also helps avoid reading the global defaults for the precision and rounding mode, together with #56095. What does this fix: * in the case of the `Irrational` constants defined in `MathConstants`: relevant methods have `@assume_effects :foldable` applied, which includes `:effect_free`, which requires that no globals be mutated (followup on #55886) * in the case of `AbstractIrrational` values in general, this PR prevents data races on the global `BigFloat` precision (cherry picked from commit 2a89f71)
1 parent b565c28 commit d04631d

File tree

2 files changed

+119
-7
lines changed

2 files changed

+119
-7
lines changed

base/irrationals.jl

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,20 +60,40 @@ AbstractFloat(x::AbstractIrrational) = Float64(x)::Float64
6060
Float16(x::AbstractIrrational) = Float16(Float32(x)::Float32)
6161
Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x))
6262

63-
function _irrational_to_rational(::Type{T}, x::AbstractIrrational) where T<:Integer
64-
o = precision(BigFloat)
63+
function _irrational_to_rational_at_current_precision(::Type{T}, x::AbstractIrrational) where {T <: Integer}
64+
bx = BigFloat(x)
65+
r = rationalize(T, bx, tol = 0)
66+
if abs(BigFloat(r) - bx) > eps(bx)
67+
r
68+
else
69+
nothing # Error is too small, repeat with greater precision.
70+
end
71+
end
72+
function _irrational_to_rational_at_precision(::Type{T}, x::AbstractIrrational, p::Int) where {T <: Integer}
73+
f = let x = x
74+
() -> _irrational_to_rational_at_current_precision(T, x)
75+
end
76+
setprecision(f, BigFloat, p)
77+
end
78+
function _irrational_to_rational_at_current_rounding_mode(::Type{T}, x::AbstractIrrational) where {T <: Integer}
79+
if T <: BigInt
80+
_throw_argument_error_irrational_to_rational_bigint() # avoid infinite loop
81+
end
6582
p = 256
6683
while true
67-
setprecision(BigFloat, p)
68-
bx = BigFloat(x)
69-
r = rationalize(T, bx, tol=0)
70-
if abs(BigFloat(r) - bx) > eps(bx)
71-
setprecision(BigFloat, o)
84+
r = _irrational_to_rational_at_precision(T, x, p)
85+
if r isa Number
7286
return r
7387
end
7488
p += 32
7589
end
7690
end
91+
function _irrational_to_rational(::Type{T}, x::AbstractIrrational) where {T <: Integer}
92+
f = let x = x
93+
() -> _irrational_to_rational_at_current_rounding_mode(T, x)
94+
end
95+
setrounding(f, BigFloat, RoundNearest)
96+
end
7797
Rational{T}(x::AbstractIrrational) where {T<:Integer} = _irrational_to_rational(T, x)
7898
_throw_argument_error_irrational_to_rational_bigint() = throw(ArgumentError("Cannot convert an AbstractIrrational to a Rational{BigInt}: use rationalize(BigInt, x) instead"))
7999
Rational{BigInt}(::AbstractIrrational) = _throw_argument_error_irrational_to_rational_bigint()

test/threads_exec.jl

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,61 @@ end
2828
# (expected test duration is about 18-180 seconds)
2929
Timer(t -> killjob("KILLING BY THREAD TEST WATCHDOG\n"), 1200)
3030

31+
module ConcurrencyUtilities
32+
function new_task_nonsticky(f)
33+
t = Task(f)
34+
t.sticky = false
35+
t
36+
end
37+
38+
"""
39+
run_concurrently(worker, n)::Nothing
40+
41+
Run `n` tasks of `worker` concurrently. Return when all workers are done.
42+
"""
43+
function run_concurrently(worker, n)
44+
tasks = map(new_task_nonsticky Returns(worker), Base.OneTo(n))
45+
foreach(schedule, tasks)
46+
foreach(fetch, tasks)
47+
end
48+
49+
"""
50+
run_concurrently_in_new_task(worker, n)::Task
51+
52+
Return a task that:
53+
* is not started yet
54+
* when started, runs `n` tasks of `worker` concurrently
55+
* returns when all workers are done
56+
"""
57+
function run_concurrently_in_new_task(worker, n)
58+
function f(t)
59+
run_concurrently(t...)
60+
end
61+
new_task_nonsticky(f Returns((worker, n)))
62+
end
63+
end
64+
65+
module AbstractIrrationalExamples
66+
for n 0:9
67+
name_aa = Symbol(:aa, n)
68+
name_ab = Symbol(:ab, n)
69+
name_ba = Symbol(:ba, n)
70+
name_bb = Symbol(:bb, n)
71+
@eval begin
72+
Base.@irrational $name_aa exp(BigFloat(2)^$n)
73+
Base.@irrational $name_ab exp(BigFloat(2)^-$n)
74+
Base.@irrational $name_ba exp(-(BigFloat(2)^$n))
75+
Base.@irrational $name_bb exp(-(BigFloat(2)^-$n))
76+
end
77+
end
78+
const examples = (
79+
aa0, aa1, aa2, aa3, aa4, aa5, aa6, aa7, aa8, aa9,
80+
ab0, ab1, ab2, ab3, ab4, ab5, ab6, ab7, ab8, ab9,
81+
ba0, ba1, ba2, ba3, ba4, ba5, ba6, ba7, ba8, ba9,
82+
bb0, bb1, bb2, bb3, bb4, bb5, bb6, bb7, bb8, bb9,
83+
)
84+
end
85+
3186
@testset """threads_exec.jl with JULIA_NUM_THREADS == $(ENV["JULIA_NUM_THREADS"])""" begin
3287

3388
@test Threads.threadid() == 1
@@ -1347,6 +1402,43 @@ end
13471402
end
13481403
end
13491404

1405+
@testset "race on `BigFloat` precision when constructing `Rational` from `AbstractIrrational`" begin
1406+
function test_racy_rational_from_irrational(::Type{Rational{I}}, c::AbstractIrrational) where {I}
1407+
function construct()
1408+
Rational{I}(c)
1409+
end
1410+
function is_racy_rational_from_irrational()
1411+
worker_count = 10 * Threads.nthreads()
1412+
task = ConcurrencyUtilities.run_concurrently_in_new_task(construct, worker_count)
1413+
schedule(task)
1414+
ok = true
1415+
while !istaskdone(task)
1416+
for _ 1:1000000
1417+
ok &= precision(BigFloat) === prec
1418+
end
1419+
GC.safepoint()
1420+
yield()
1421+
end
1422+
fetch(task)
1423+
ok
1424+
end
1425+
prec = precision(BigFloat)
1426+
task = ConcurrencyUtilities.new_task_nonsticky(is_racy_rational_from_irrational)
1427+
schedule(task)
1428+
ok = fetch(task)::Bool
1429+
setprecision(BigFloat, prec)
1430+
ok
1431+
end
1432+
@testset "c: $c" for c AbstractIrrationalExamples.examples
1433+
Q = Rational{Int128}
1434+
# metatest: `test_racy_rational_from_irrational` needs the constructor
1435+
# to not be constant folded away, otherwise it's not testing anything.
1436+
@test !Core.Compiler.is_foldable(Base.infer_effects(Q, Tuple{typeof(c)}))
1437+
# test for race
1438+
@test test_racy_rational_from_irrational(Q, c)
1439+
end
1440+
end
1441+
13501442
@testset "task time counters" begin
13511443
@testset "enabled" begin
13521444
try

0 commit comments

Comments
 (0)